Никогда не пишите длинных if-ов

    Ошибок в условиях допускается великое множество. Можно взять для примера любой пост из блога PVS-studio, в каждом есть ошибки, связанные с невнимательным обращением с условиями. И правда, нелегко разглядеть ошибку в условии, если код выглядит так (пример из этого поста):

    static int ParseNumber(const char* tx)
    {
      ....
      else if (strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0
        || strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0
        || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0
        || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0
        || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0
              ))
      {
          return 4;
      }
      else if (strlen(tx) >= 3
        && (strncmp(tx, "+%e", 3) == 0
         || strncmp(tx, "-%e", 3) == 0
         || strncmp(tx, "%pi", 3) == 0   // <=
         || strncmp(tx, "Nan", 3) == 0
         || strncmp(tx, "Inf", 3) == 0
         || strncmp(tx, "%pi", 3) == 0)) // <=
      {
          return 3;
      }
      ....
    }

    Такие условия явление повсеместное, я сталкивался с ними абсолютно во всех проектах с которыми я имел дело. Вот этот пост на хабре отлично иллюстрирует сложившийся в программистском мире зоопарк того, как «можно» писать условия для if-else и каждый подход аргументирован и по своему крут, вообще пробелы рулят, табуляция отстой и все такое. Самое смешное, что он начинается словами «Условный оператор в обычной своей форме источником проблем является сравнительно редко», за подтверждением обратного отсылаю вас, опять же, к блогу PVS-studio и вашему собственному горькому опыту.

    Я заметил, что в последнее время при написании if-else блоков начал повсеместно использовать подход, который формализовал только сейчас и захотел с вами поделиться. В общем и целом он совпадает с описанным в первом же комментарии к вышеупомянутому посту, но радикальней и с четкой идеей. Я использую такую технику вообще во всех условиях, не важно, сложные они или простые.

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

    По правде, второе даже важнее. Если по поводу условия есть, что сказать, кроме того, что явно написано в нем самом, нужно подумать, можно ли это выразить названием переменной. Сравните два псевдокода:

    if (model.user && model.user.id) {
        doSomethingWithUserId(model.user.id);
        ...
    }

    и

    let userExistsAndValid = model.user && model.user.id;
    if (userExistsAndValid) {
        doSomethingWithUser(model.user);
        ...
    }

    В первом случае у нас чисто формальная проверка значений, нам нужен user.id и мы проверяем, есть ли он, все можно оставить как есть. Во втором случае нам нужна уже валидная модель и мы объясняем это в имени переменной через термины бизнес-логики.

    Такой подход имеет преимущества и при рефакторинге: если проверка понадобится дальше в методе еще раз и/или расширятся условия валидности (нужно будет, чтобы у юзера еще и емейл был обязательно) — изменения будут минимальны.

    Возьмем пример тоже про пользователя, но посложнее, с которым я столкнулся буквально несколько дней назад и отрефакторил в этом стиле. Было:

    if (this.profile.firstName && this.profile.lastName &&
        (this.password ||
         !!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId)) {
       ....
    }

    Стало:

    const hasSlack = !!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId);
    const hasSomeLoginCredentials = this.password || hasSlack;
    const hasPersonalData = this.profile.firstName && this.profile.lastName;
    if (hasPersonalData && hasSomeLoginCredentials) {
       ....
    }

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

    Для меня в списке исключений стоят еще шорткаты, очевидно, если надо просто выразить что-то вроде:

    if (err || !result || !result.length === 0) return callback(err);

    нет смысла вводить переменные.
    Поделиться публикацией
    Похожие публикации
    AdBlock похитил этот баннер, но баннеры не зубы — отрастут

    Подробнее
    Реклама
    Комментарии 140
    • +2
      Круто. Спасибо. Действительно Ваш вариант читается лучше, а длинные if встречаются практически везде.
      • +1
        Надо более активно использовать контейнеры с групповыми операциями, а так же переходить на языки с поддержкой pattern matching.
        • +7
          не ново, но очень полезно
          • –25

            Когда сам пишешь длинные if'ы, то никогда в них не ошибаешься. Статью надо назвать:


            Никогда не пишите копипастите длинных if-ов

            А логика вида a=(b>3?(c<3?b+2:2):5); и сложнее, это отдельное искуство!

            • +4

              Искусство искусством, но когда бизнес меняется и через пол года год, тебя просят подправить бизнес логику в подобном выражении a=(b>3?(c<3?b+2:2):5) волосы встают дыбом! и тихо начинаешь себя ненавидеть и проклинать за подобный когда-то написанный код. :) поэтому всегда стараюсь использовать человеко понятные имена переменных и примерно как в статье агрегировать условия в более понятные и упорядочены блоки

              • +7

                А что в этом выражении может вызвать затруднение?


                Что касается статьи… там так и не раскрыто, что делать с исходными if-ами.


                if (strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0
                    || strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0
                    || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0
                    || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0
                    || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0
                          ))
                  {
                      return 4;
                  }

                предлагается заменить на:


                int iDontKnowWhatIsItButMustCauseReturningFour = strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0
                    || strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0
                    || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0
                    || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0
                    || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0;
                if (iDontKnowWhatIsItButMustCauseReturningFour) return 4;

                Стало понятнее, правда? :)


                Я бы в таком случае вынес "страшное" условие в функцию/метод и его документировал. Заодно можно и юнит-тест к нему написать, чтобы уж совсем не оставить места для ошибки.

                • 0

                  Тут стоит ввести ещё 5 временных переменных. А лучше вообще ограничиться одной регуляркой.

                  • 0
                    Окей, просмотреть условие или просмотреть переменную. Без обид, это обыкновенное перекладывание шаров из урны в урну. А одна регулярка — это частный случай под конкретную задачу, и то не всегда осуществимый.
                    • 0
                      Основная цель, которую ставит перед собой автор, — повысить читаемость и понимание кода, когда Вы в него окунетесь снова, скажем через год. Я бы, наверное, тоже разбил это условие на 5 частей и дал бы каждому понятное имя.
                      • 0
                        Да, возможно.
                        Но, опять же, есть «сложные» задачи и есть «большие» задачи. И с теми, и с другими напряжно работать. Но если в «сложной» задаче есть возможность выделить условие в переменную с понятным названием, с большими всё не так. Вот тут, например, хоть как ты изворачивайся, у тебя в любом случае получится плохо. Да, можно разбить на предпроверку переменных, проверку типа и тд. Но чем больше делишь, тем больше получается сущностей. Чем больше имён, тем больше внимания уходит на них, и тем больше вероятности допустить ошибку.

                        Да, я бы тоже переписал это условие, разбив на куски поменьше. Избавился от волшебных чисел, дав имена константам и определив тракториста к балансу. Но это условие не сделаешь простым для понимания, оно всё равно будет сложным. Большим.
                        • –1
                          Вот тут, например, хоть как ты изворачивайся, у тебя в любом случае получится плохо

                          Ну вы ведь ошибаетесь. Там есть 11 логически-независимых условий. Каждое из них — отдельный метод. А потом 12 метод, который их объединяет. Получится хорошо и понятно.
                          • –1
                            Я разве написал, что нельзя определить 11 методов и 12-й, их объединяющий? Нет.
                            Я написал, что вы вдобавок к имеющимся потрашкам добавляете 12 имён. Которые далеко не всегда отличаются лаконичностью и информативностью. И это не только 12 имён, это ещё и 12 сигнатур. Это всё те же 11 условий, но разбросанные по исходникам так, что теперь они не помещаются на один экран. Как итог, у вас остаются всё те же 11 условий в том же виде, но они размотаны по коду, и весь if теперь нужно не только целиком понять, но, для начала, собрать в одном месте.

                            Отрежу сразу, я не сторонник запихивания всего и вся в одно условие. Но нужно помнить и о бритве Оккама: сущностей должно быть ровно столько, сколько необходимо и достаточно. Если проверка баланса встречается больше 2 раз, её можно и нужно сделать методом. Если тип проверяется на неравенство постоянно, нужно добавить соответствующий метод. Если из типов постоянно получаются характеристики, нужно это так же оформить в методы.
                            Но если все проверки этого слонопотама уникальны, то не нужно усложнять свою жизнь. Один раз документировали и оставили на суд потомкам.
                            • 0
                              весь if теперь нужно не только целиком понять, но, для начала, собрать в одном месте.

                              А зачем? Важно понять каждый блок по отдельности (анализ) и блок в целом (синтез), но не всё вместе (понять блок как огромное количество мелких сущностей). Когда вы думаете про разработку большого ПО — вы ведь не мыслите про все его самые мелкие операции одновременно. Вы имеете в своей оперативке самые глобальные абстракции, как они влияют одна над другую и знаете их влияние на конкретный маленький блок, необходимый в момент разработки.
                              • 0
                                Важно понять каждый блок по отдельности (анализ) и блок в целом (синтез), но не всё вместе (понять блок как огромное количество мелких сущностей).
                                Это всё хорошо на лекции по ООП или сисану. А в конкретной программе это всё может не иметь смысла. И, скорее всего, не будет иметь.

                                Потому как вы идёте в старый код не из праздного интереса и не из желания прочитать труды классиков. У вас либо баг, либо модификация. Ни в том, ни в другом случае вам не нужна цепочка методов длиной в стек истории переходов. Вам нужно быстро попасть на место, быстро найти всех виновных, быстро же раздать всем витамина Р, и быстро же забыть об этом. Проблема с вереницей методов в том, что виновные далеко друг от друга.
                                • 0
                                  Это всё хорошо на лекции по ООП

                                  Этот подход не специфичен для какой-либо парадигмы

                                  Проблема с вереницей методов в том, что виновные далеко друг от друга.

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

                    if (
                           (typeId != 4 && typeId != 5 && showBalance != 2 && !(showBalance == 0 ? balance >= -300 : balance < -300))
                        || (typeId != 4 && typeId != 5 && showAct != -1 && !(showAct == act))
                        || (typeId != 4 && typeId != 5 && showDog != -1 && !(showDog == dog))
                        || (typeId != 1 && typeId != 4 && typeId != 5 && showFullAcc != 2 && !(showFullAcc == 1 ? fullAcc != 0 : fullAcc == 0))
                        || (showToPay != 2 && !(showToPay == toPay))
                        || (showFix != 100 && !(showFix == fix))
                        || (showComment != 2 && !(showComment == 1 || showComment == 0 ? (showComment == 1 ? cc > 0 : (cc > 0 && ccn > 0)) : (showComment == 10 ? cs > 0 : (cs > 0 && ccn > 0))))
                        || (!isEmpty(showManagers) && (!(showManagers.find(function (a){ return a == manager}) != undefined) == selectedMangers))
                        || (!isEmpty(showServices) && (!(showServices.find(function (a){ return a == services}) != undefined) == selectedServices))
                        || (!isEmpty(showTypeServices) && (!(showTypeServices.find(function (a){ return a == typeId}) != undefined) == selectedTypes))
                        || (!isEmpty(firmIds) && !(firmIds.find(function (a){ return a == fId}) != undefined))
                    ) {
                        return false;
                    }
                    
                    • +2

                      Первое, что я бы сделал — это разбил один большой if на много маленьких, избавившись от ||.
                      Второе — для повторяющихся условий типа typeId != 4 && typeId != 5 завёл бы по переменной.
                      Третье — максимально упросил условия: куда проще читать showAct != act, чем !(showAct == act).


                      И напоследок: выражение !(showManagers.find(function (a){ return a == manager}) != undefined) == selectedMangers) мне абсолютно не понятно. Результатом showManagers.find(function (a){ return a == manager}) != undefined) является bool, который сравнивается с selectedMangers, который, судя по имени, имеет явно не булевый тип, и остаётся только гадать, что имелось в виду в коде.

                      • +6
                        но спустя время придется напрячься чтобы припомнить что тут и зачем

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

                        оригинал
                        if (
                               (typeId != 4 && typeId != 5 && showBalance != 2 && !(showBalance == 0 ? balance >= -300 : balance < -300))
                            || (typeId != 4 && typeId != 5 && showAct != -1 && !(showAct == act))
                            || (typeId != 4 && typeId != 5 && showDog != -1 && !(showDog == dog))
                            || (typeId != 1 && typeId != 4 && typeId != 5 && showFullAcc != 2 && !(showFullAcc == 1 ? fullAcc != 0 : fullAcc == 0))
                            || (showToPay != 2 && !(showToPay == toPay))
                            || (showFix != 100 && !(showFix == fix))
                            || (showComment != 2 && !(showComment == 1 || showComment == 0 ? (showComment == 1 ? cc > 0 : (cc > 0 && ccn > 0)) : (showComment == 10 ? cs > 0 : (cs > 0 && ccn > 0))))
                            || (!isEmpty(showManagers) && (!(showManagers.find(function (a){ return a == manager}) != undefined) == selectedMangers))
                            || (!isEmpty(showServices) && (!(showServices.find(function (a){ return a == services}) != undefined) == selectedServices))
                            || (!isEmpty(showTypeServices) && (!(showTypeServices.find(function (a){ return a == typeId}) != undefined) == selectedTypes))
                            || (!isEmpty(firmIds) && !(firmIds.find(function (a){ return a == fId}) != undefined))
                        ) {
                            return false;
                        }
                        


                        function has (array, needle) {
                        	return !isEmpty(array) && array.includes(needle);
                        }
                        
                        const TYPE_QUX = 1;
                        const TYPE_FOO = 4;
                        const TYPE_BAR = 5;
                        
                        
                        if ([TYPE_FOO, TYPE_BAR].contains(typeId) == false) {
                        	if (showBalance == 0 && balance >= -300) {
                        		return false;
                        	}
                        	
                        	if (showBalance != 0 && showBalance != 2 && balance < -300) {
                        		return false;
                        	}
                        	
                        	if (showAct != -1 && showAct != act) {
                        		return false;
                        	}
                        	
                        	if (showDog != -1 && showDog != dog) {
                        		return false;
                        	}
                        }
                        
                        
                        if ([TYPE_FOO, TYPE_BAR, TYPE_QUX].contains(typeId) == false) {
                        	if (showFullAcc == 1 && fullAcc == 0) {
                        		return false;
                        	}
                        	
                        	if (showFullAcc != 1 && showFullAcc != 2 && fullAcc != 0) {
                        		return false;
                        	}
                        }
                        
                        // ...
                        
                        if (has(showManagers, manager) != selectedMangers) {
                        	return false;
                        }
                        
                        if (has(showServices, services) != selectedServices) {
                        	return false;
                        }
                        
                        // ...
                        


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

                        На самом деле проблема в том, что программист, который это писал вместо того, чтобы думать, как написать код понятнее думал, какой он (программист) охуенный, что может писать такой херовый код.
                        • 0

                          Немного упростил:


                          function has( array , needle ) {
                              if( isEmpty( array ) ) return false
                              return array.includes( needle )
                          }
                          
                          const TYPE_QUX = 1
                          const TYPE_FOO = 4
                          const TYPE_BAR = 5
                          
                          if( ![ TYPE_FOO , TYPE_BAR ].contains( type_id ) ) {
                          
                              check_ballance:
                              if( balance >= -300 ) {
                                  if( show_balance == 0 ) return false
                              } else {
                                  if( show_balance == 0 ) break check_ballance
                                  if( show_balance == 2 ) break check_ballance
                                  return false
                              }
                          
                              if( show_act != -1 && show_act != act ) return false
                              if( show_dog != -1 && show_dog != dog ) return false
                          
                              check_full_acc:
                              if( TYPE_QUX != type_id ) {
                          
                                  if( show_full_acc == 1 ) {
                                      if( full_acc == 0 ) return false
                                  } else {
                                      if( show_full_acc == 2 ) break check_full_acc
                                      if( full_acc == 0) break check_full_acc
                                      return false
                                  }
                          
                              }
                          
                          }
                          
                          // ...
                          
                          if( has( show_managers , manager ) != selected_mangers ) return false
                          if( has( show_services , services ) != selected_services ) return false
                          
                          // ...
                          • +1
                            ИМХО это не упрощение, а последний абзац того комментария, на который вы ответили
                            • 0

                              И что же тут у вас вызывает сложности?

                      • 0
                        Я не очень в C но в PHP я скорей всего сделал бы так

                        ........
                        $needles[4] = ["+%pi", "-%pi", "+Inf", "-Inf", "+Nan", "-Nan", "%nan", "%inf"];
                        $needles[3] = ["+%e", "-%e", "%pi", "Nan", "Inf", "%pi"];
                        .......
                        foreach ($needles AS $length => $needle) {
                            if  (mb_strlen($tx) < $length) continue;
                            $haystack = mb_substr($tx, 0, 4)
                            if (in_array($haystack, $needle)) {
                                return $length;
                            }
                            return 0;
                        }
                        
                        


                        Думаю что в C можно что-то подобное
                        • 0
                          не могу редактировать свои коментарии
                              $haystack = mb_substr($tx, 0, $length)
                          
                          • 0
                            $needles[3] = ["+%e", "-%e", "%pi", «Nan», «Inf», "%pi"];
                            Вы с радостью заглотили ту же ошибку.
                            • 0
                              Да, я как-от не вникал в набор данных, просто скопипастил :(

                              Просто хотел избавится от лишних блоков else if
                              • +1
                                Есть очень хорошая игра для компании. Keep talking and nobody explodes. Скачайте мануал и посмотрите, сможете ли вы избавиться от вереницы лишних if-else.

                                Статья написана о частных случаях. В частном случае можно было вообще проверять посимвольно. Но, в общем, не ясно, что лучше: вереница if'ов или вереница переменных с промежуточными результатами. Или именованные методы с проверкой.

                                Моё мнение, что каждый отдельный случай требует своего подхода. Где-то лучше впихнуть всё в один if
                                if (object 
                                 && object->isValid() 
                                 && object->hasAnimation() 
                                 && object->isNotAnimatedNow())  {
                                    object->animate();
                                }
                                

                                а где-то стоит разнести по переменным
                                string filename1 = FileSystemPath(file1->path()).makeAbsolute().string();
                                string filename2 = FileSystemPath(settings.load(KEY).toString()).makeAbsolute().string();
                                if (compare(filename1, filename2, FileSystemPath::CaseSensitivity())) {
                                  DoSomething();
                                }
                                
                          • 0

                            Можно-то можно, но ведь оно так не стало понятнее. Зато, как уже сказали, стало менее оптимизированно. Лучше уж в регэксп, действительно, переделать. Скомпилированный.

                          • 0
                            В данном случае лучше вынести все строковые константы на сравнение в отдельный массив и итерироваться по нему.

                            Но универсальных решений все равно нет — вышеприведенный пример с большим if-ом, к примеру, имеет максимальное быстродействие, возвращая 4 сразу, как только найдет нужную строчку.
                            • 0

                              Третий раз предлагают массив с иттерациями как лучшее решение. Очень странно. Оно хуже, чем if-ы по всем параметрам. Не более понятно и хуже по быстродействию. А если уж так сильно хочется использовать какой-нибудь контейнер, то гораздо лучше запихать их в хешмапу.

                              • 0
                                По быстродействию хуже, но совсем ненамного — на один цикл и немного арифметики указателей.
                                Стандартная хешмапа для такого маленького набора строк ни разу не факт, что быстрее — вычисление хеша дороже, чем сравнение строк. Еще memory locality — хешмапа в плюсах построена на односвязных списках или с открытой адресацией?
                                • 0

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

                            • 0
                              Просто нужно понять, что есть, например

                                  || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0
                                  || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0
                                  || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0
                              


                              И выделить этот блок в отдельную функцию, назвав ее наиболее точно, например

                              bool isValueNonRationalNumeric(const std::string& _value)
                              {
                                return strncmp(_value.c_str(), "+Inf", 4) == 0 || strncmp(_value.c_str(), "-Inf", 4) == 0 ||
                                       strncmp(_value.c_str(), "+Nan", 4) == 0 || strncmp(_value.c_str(), "-Nan", 4) == 0 ||
                                       strncmp(_value.c_str(), "%nan", 4) == 0 || strncmp(_value.c_str(), "%inf", 4) == 0;
                              }
                              
                              • –1

                                а что мешает вынести


                                bool areStringsEqual(const std::string& _value, const char*const another) {
                                  return strncmp(_value.c_str(), another, strlen(another)) == 0; 
                                }
                                • +1

                                  Тогда уж


                                  template<int N> bool areStringsEqual(std::string const& value, char const (&another)[N]) {
                                      return strncmp(value.c_str(), another, N) == 0;
                                  }
                        • 0
                          И этому искусству место на выставках, а не в продакшене.
                        • –15

                          Есть проблемы.
                          let userExistsAndValid = model.user && model.user.id; if (userExistsAndValid) { doSomethingWithUser(model.user);
                          В многопоточном коде не работает. Был валидный, к следующей строке стал не очень. В сетевом — тоже. При неаккуратном рефакторинге между первой и второй строчкой может вклиниться удаление юзера. Использование переменной userExistsAndValid как-бы побуждает использовать её значение в дальнейшем. В то время как её нужно каждый раз перевычислять. Уж если так делать, то нужно её после блока if сразу сбрасывать в некоторое значение "ни да ни нет".

                          • +4
                            Статья же не об этом.
                            Как бы очевидно, что если есть многопоточный код, то в нем есть и транзакции/мутексы/как_оно_называется_в_вашем_языке, в которых и пишешь то, куда никто не должен вклиниваться.
                            • +30
                              Простите, а разве сложное-навороченное выражение выполняется как атомарная операция? Ведь между операциями чтения model.user и model.user.id тоже может вклиниться удаление юзера.
                              • –1
                                В данном случае model.user это структура с полем id, которая была атомарно получена из хранилища. Не вижу тут никаких проблем. Видимо, подразумевается, что хранилище возвращает либо модель либо null, false… Поэтому сначала проверка на то, является ли результат собственно структурой.
                                • 0

                                  Независимо от того, является ли model.user разделяемой или нет, потоко(не)безопасность от наличия лишней переменной не зависит. Для разделяемого поля оба варианта будут содержать гонку, для неразделяемого (ваш случай) оба варианта будут безопасными.

                                  • 0
                                    Ой, я думал это коммент к статье, не сразу понял при чем тут потоки:) Так-то да.
                              • +2
                                {
                                  bool userExistsAndValid = model.user && model.user.id; 
                                  if (userExistsAndValid) 
                                    { doSomethingWithUser(model.user);
                                }
                                

                                Ну а про потоки вы вообще бред несете
                                • +5

                                  А что помешает пользователю перестать быть вадидным после проверки в первой строчке и перед вызовом doSomethingWithUserId во второй строчке, в этом коде?


                                  if (model.user && model.user.id) {
                                      doSomethingWithUserId(model.user.id);
                                  }
                                  • +8

                                    То есть если код помещается в одну строчку, то он выпоняется атомарно?

                                  • +7

                                    Статья, конечно, полезная, но есть ощущение, что просто пересказана одна из страничек книги господина Макконнелла.


                                    Так что всем тем, кому понравился данный топик — настоятельно рекомендую к прочтению эту чудотворную книгу. Там не только про подобные условия есть, а вообще много чего на тему читаемого кода.

                                    • +1
                                      Я её уже 11 лет не могу полностью осилить…
                                      Может время пришло… Попробую еще разок.
                                      • +1

                                        я бегло прошелся по Макконнеллу перед написанием статьи (блин, три сдвоенных согласных это круто) он, безусловно, пишет про самодокументирующийся код, смысловые названия переменных и все такое, но на условиях он не делает какого-то отдельного акцента на условиях.


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

                                        • 0

                                          "Макконнелловскими принципами"? Кажется, это принципы логики, не? =)

                                          • 0
                                            Я, может быть, не до конца понимаю разницу, но очень похожие рекомендации я встречал в его книге как минимум несколько раз (сейчас как раз понемногу читаю).

                                            Как пример, подраздел «Последовательности операторов if-then-else» раздела «15.1. Операторы if». Как раз про то, что нужно сворачивать множественные условия в единичные функции проверки для удобства чтения и понимания.

                                            Еще пример — подраздел «Упрощение сложных выражений» раздела «19.1. Логические выражения» — о выведении дополнительных переменных, сокращающих количество условий методом группировки по логическому соответствию.

                                            Как по мне — очень похоже на содержимое статьи.
                                          • 0
                                            +1. И «Рефакторинг» Фаулера в придачу. В частности: https://refactoring.com/catalog/decomposeConditional.html
                                          • +3
                                            Первый пример просто чудовищный, я бы с таким человеком в одном коворкинге кодить не сел. Просто когда рука поднимается такое писать, ну это уже всё, приехали. Вот хотя бы так, что ли:
                                            static int ParseNumber(const char* tx)
                                            {
                                              static const char* Is4[]= {"%eps", "+%pi", "-%pi", "+Inf", "-Inf", "+Nan", "-Nan", "%nan", "%inf", NULL};
                                              static const char* Is3[]= {"+%e", "-%e", "-%pi", "%pi", "Nan", "Inf", "%pi", NULL};
                                              ....
                                              else if (contains(Is4, tx))
                                              {
                                                  return 4;
                                              }
                                              else if (contains(Is3, tx))
                                              {
                                                  return 3;
                                              }
                                              ....
                                            }
                                            
                                            • +1
                                              В исходном коде явно видна попытка оптимизировать по скорости (проверка strlen перед перебором), ваш же вариант прямо противоположен (включая и то что функция contains будет медленнее чем захардкоденые проверки).
                                              • –1
                                                Ту странную проверку на длину можно и сюда добавить, а вот «функция contains будет медленнее чем захардкоденые проверки» — вы уверены, что будет какое-либо значительное увеличение скорости?
                                                • +1
                                                  Разница конечно маленькая (как от strlen так и от реализации сравнений), но всё же именно она могла быть причиной того кода. Хотя может это и просто первый пришедший на ум вариант кого-то, кто об этом вообще не думал.
                                                • 0
                                                  Тогда можно запихнуть проверки в макросы. Будет более читаемо и сохранится возможность оптимизаций компилятором.
                                                  • 0
                                                    ничего не мешает добавить минимальный ожидаемый strlen аргументом в contains или перед ним такую же проверку сделать, суть не в этом же.
                                                    if (strlen(tx) > 3 && contains(...))
                                                  • 0

                                                    если производительность не критична, то да, я тоже считаю, что как-то так и надо делать, все понятно. В противном случае можно оставить все как есть, но свести в итоге все к


                                                      else if(isThreeCharsSpecialSymbol(tx))
                                                      {
                                                          return 3;
                                                      }
                                                      else if (isFourCharsSpecialSymbol(tx))
                                                      {
                                                          return 4;
                                                      }

                                                    Или даже написать isNCharsSpecialSymbol(tx, n) и переделать все в цикл :)

                                                    • 0
                                                      Что разницы, что вы всё переделали по-своему, если вы затянули ошибку из того кода?
                                                      static const char* Is3[]= {"+%e", "-%e", "-%pi", "%pi", «Nan», «Inf», "%pi", NULL};
                                                      Что толку бить себя в грудь и рвать тельняшку, если вы тут же ловким движением обсираетесь? Ладно, автор оригинального кода протупил при копипасте, вам же специально подсветили «вот они, ошибки, не делайте их», и вы с размаху их делаете.

                                                      При этом проверка одинаковых условий анализатором куда проще, чем проверка содержимого массивов. При этом, компилятор может выплюнуть лишнюю проверку. При этом, будь у вас не 6 блоков строчек, а 600, что ваш, что тот код были бы одинаково плохо читаемы, потому что много буков. Но об этом мы не думаем, у нас на конкретной задаче мир закончился, и океан упирается в край земли.
                                                      • 0
                                                        Это был псевдокод, если вы не поняли, и суть была не в устранении ошибки. Я вам ещё намекну на будущее, функция contains нигде не определена, пример даже не скомпилится, т.е. я получается 2 раза обосрался, пользуясь вашей терминологией.
                                                        Но тут есть нюанс, кто-то постоянно кладет себе в штаны, это даже может быть незаметно со стороны, пока не потечет (правда code smell при ревью кода, конечно сшибает с ног), а кто-то понимает, что так делать не надо. А с виду оба одеты, в штанах, оба программисты.
                                                        • 0
                                                          Не соскакивайте с темы. Ваш пример ни капельки не спасает от человеческого фактора. Вы, писавший, что код нужно переписать иначе, чтобы было проще обработать условие, тут же оставляете ошибку старого кода.

                                                          И какая мне, тимлиду, разница, как вы сделали проверку: через массив или перебором в строчку, если ошибка как была, так и осталась?

                                                          Я не оправдываю тот код, в нём достаточно феерии, но чем вы лучше? Разве что рассуждениями о штанах.
                                                          • 0
                                                            А я вам больше скажу, ничего не спасет от человеческого фактора. Вы сейчас настолько мощно путаете теплое с мягким, что я даже не знаю как реагировать, похоже на жирный троллинг. Ну это как, я не знаю, сравнивать двух водителей, один раздолбай, гоняет, не пристегивается, второй аккуратный, вежливый. В итоге обоих в какой-то момент разматывает об встречный грузовик (ну бывает же, даже с самыми аккуратными, при стечении определенных обстоятельств, согласитесь). И тут выходите вы, весь в белом, и с укоризной вещаете, дескать что толку что он такой весь аккуратный был, вон теперь только кишки по асфальту.
                                                            • 0
                                                              Я не заметил аккуратного вежливого водителя в ветке.
                                                    • +7
                                                      Я конечно не программист, но спрошу: почему не вынести проверку в отдельную функцию с понятным названием? А в случае с user — в отдельный метод?
                                                      • +1
                                                        Если просто пытаться объяснить, то все сложно )
                                                        Вся статья (имхо) сводится к: пишите код проще, проще для понимания.

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

                                                        Минус — ну это очевидно — код становиться большим, но как-бы читаемым, и снова (блин) сложным.
                                                        • +2
                                                          Если условие необходимо проверять во многих местах, так и делают. Вместо переменной
                                                          let userExistsAndValid = model.user && model.user.id;
                                                          вводят функцию
                                                          function userExistsAndValid { return model.user && model.user.id; }
                                                          • 0
                                                            Скорее всего, тогда будет или куча новых мелкых методов, которые опять же придётся сравнивать, либо
                                                            function x(){
                                                            if(x == 1) {
                                                            ...
                                                            }
                                                            }

                                                            станет

                                                            function x(){
                                                            if(y()) {
                                                            ...
                                                            }
                                                            }
                                                            function y(){
                                                            return ... && ... && ... && ... && ... && ... && ... && ... && ... && ..;
                                                            }


                                                            Что не то чтобы проще
                                                          • +6
                                                            В первом случае у нас чисто формальная проверка значений, нам нужен user.id и мы проверяем, есть ли он, все можно оставить как есть. Во втором случае нам нужна уже валидная модель и мы объясняем это в имени переменной через термины бизнес-логики.

                                                            В том-то и дело, что по логике нам тут нужна не "валидная модель", а "модель с идентификатором". Валидность может включать в себя проверку кучу условий по разным полям, которые тут не нужны. Вы могли бы назвать эту переменную "hasUserWithId", но это ничем не лучше "model.user?.id". Отсюда вывод: вводить переменную с говорящим названием нужно лишь тогда, когда у вас не получается написать говорящий код :-)

                                                            • +2
                                                              Отсюда вывод: вводить переменную с говорящим названием нужно лишь тогда, когда у вас не получается написать говорящий код :-)

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


                                                              Что говорит о том, что введение таких переменных это попытка лечить симптомы в коде, а не его архитектуры.
                                                              Если у модели или сущности нет метода IsValid(), то вынесение простыней условий в якобы "говорящие" переменные только засоряет код и ведет в side-эффектам из-за того, что часть условий вычисляется, когда это не нужно.

                                                              • 0

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

                                                            • +13

                                                              Предложенный подход тоже не лучший.


                                                              Когда плодятся локальные переменные вида "let userExistsAndValid = model.user && model.user.id",
                                                              это признак того, что userExistsAndValid должен быть вынесен в отдельный метод, функцию, extension, и т.д.


                                                              Они засоряют код, ведут к копипасте (в местах, где нужно выполнить такую же проверку; а что вы будете делать, когда в 10 местах по всему проекту проверяются на null user и user.id, и вдруг для проверки нужно проверить третий параметр?).


                                                              Должно быть что-то вроде такого: user.IsValid()
                                                              Например, в C# это можно сделать с помощью extension, который заодно проверил user и на null тоже.
                                                              Способов полно во всех языках для этого.


                                                              "Конечно есть случаи, когда какие-то элементы условия надо выполнять только, если предыдущие были успешны и другие исключения"
                                                              А эту проблему нужно решать: если какое-то условие проверяется, когда оно не нужно, то это не только в целом "неправильно": вычисление в таком случае может проводиться на неконсистентных для контекста данных, что может привести к side-эффектам, крешам, и т.д.
                                                              Решать можно, вставляя в проверку условия лямбду с кодом, который вычисляет значение.
                                                              При необходимости можно использовать Lazy.

                                                              • 0

                                                                спасибо, все это верно. Я просто старался примеры подобрать с использованием самых простых инструментов. Сам активно использую и C#, и JS, поэтому активно пользуюсь всеми упомянутыми вами инструментами. И user.IsValid() имеет такую же ценность как userExistsAndValid в плане code quality, особенно если у вас одно и то же понятие IsValid в десяти разных местах кода. А вот если это выливается в повсеместные if (user.IsValid() && user.lastName..., то, возможно, каждый раз стоит иметь перед глазами всю цепочку логических операций.


                                                                Я тут не пришел к вам со светочем абсолютной истины, просто вот придумал делать так, понравилось, выразил в словах и написал на Хабр :) Не думаю, что может быть какой-то однозначно "лучший" подход вообще

                                                                • 0

                                                                  Я хотел донести мысль, что само по себе использование промежуточных локальных переменных не решает проблему, а в чем-то даже усугубляет (лишние вычисления, в т.ч. с возможными side-эффектами), и вообще напоминает стиль "кодинга" эдак из 80-90-х.


                                                                  В то же время с длинными условиями надо что-то делать.
                                                                  Как вариант, предлагаю в качестве промежуточных переменных использовать лямбды (или, если говорить о C# 7- локальные функции) — т.е., в месте объявление переменной не вычислять ее значение, а задать правила вычисления.
                                                                  Если какая-то переменная встречается в длинном условии более одного раза, то лямбду обернуть в Lazy.


                                                                  При соблюдении пары этих правил сохраняется преимущество предложенного вами подхода, и устраняются возможные side-эффекты.


                                                                  А если совсем по-хорошему, то, как правило, появление длинных условий свидетельствует, скорее, о то, что на этапе проектирования было сделано что-то не так.

                                                                • 0
                                                                  Вы говорите о несколько другом случае. Длинные условия — это одно. Повторяющиеся условия — это другое. Длинные условия (метрика автора мне вполне нравится — более 2 операторов) надо выносить в переменную. Иначе интерфейс рискует быть засраным по самое не могу почем зря. Да и локальность кода сохранится. Повторяющиеся условия — выносить в методы/функции (и тут уже не важно, насколько это условие длинное).
                                                                  • 0

                                                                    Вы правы насчет того, что не нужно выносить в интерфейс (контракт) то, что не имеет смысла на уровне контракта, а имеет смысл только в контексте какого то метода.

                                                                • +4
                                                                  А как быстро выполняется
                                                                  !!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId)
                                                                  
                                                                  ?
                                                                  Если hasPersonalData ложно (нет имени или фамилии) и пароля тоже нет, то в исходном варианте на этом всё заканчивается, а в новом — всё равно вызывается !!_.find.
                                                                  • –1
                                                                    ну это же просто underscore helper, его можно было бы легко заменить на [].some(), просто я оставил все как было, кроме того, что непосредственно к статье не относится. Ну а массив this.serviceAccounts, как может быть можно догадаться из контекста, редко бывает больше 10 элементов.
                                                                    • +1
                                                                      Это частный случай. В общем случае, вычисление одного из условий может быть ресурсоемким. По-моему, лучше заменять сложные условия не на переменные, а на функции/методы, чтобы не производить лишних вычислений
                                                                  • +14

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


                                                                    И эти люди нам пытаются продать анализатор кода.

                                                                    • +2
                                                                      Поспорю немного с вами.

                                                                      Вынос всех этих вычислений до цепочки, во-первых, не всегда возможен

                                                                      Так автор же об этом и пишет, нет?:

                                                                      Конечно есть случаи, когда какие-то элементы условия надо выполнять только, если предыдущие были успешны и другие исключения…


                                                                      … во-вторых, может дать проблемы с производительностью из-за вычисления кучи всего разного

                                                                      Читаемость кода и производительность (мне кажется) в большинстве случаев находятся по разные стороны.
                                                                      • +2
                                                                        Читаемость кода и производительность (мне кажется) в большинстве случаев находятся по разные стороны.

                                                                        Но данный совет может привести к деградации производительности на несколько порядков (особенно если где-то в конце цепочки else if идёт запрос к базе, а в предыдущих его нет) при весьма сомнительном улучшении читаемости в стиле паскалевского "объявить все переменные в начале процедуры, так всем понятнее будет".

                                                                        • 0
                                                                          Ага, с этим не поспоришь. Если все делать идеально читаемым, программа станет мее-е-едленным динозавром. С минутными, а то и более задержками.
                                                                          • +1

                                                                            Это неправда и любимая отмазка write-only писателей.

                                                                            • 0
                                                                              Вообще-то это гипербола. Или вы имеете ввиду быстроту написания кода, а не его выполнение?

                                                                              Пример:
                                                                              Самые быстрые куски кода пишутся на asm иногда, или похоже на asm, без объектов, вызовов функций, лямбд и т.д.

                                                                              Что вы думаете?
                                                                              • 0
                                                                                Самые быстрые куски кода пишутся на asm иногда, или похоже на asm, без объектов, вызовов функций, лямбд и т.д.
                                                                                static inline спасут от боязни выделять код в функции. И еще внезапно иногда лучше noinline в т.ч. и по производительности.
                                                                                И так говорите будто на более низкоуровневых языках (без лямбд и объектов) нельзя писать читаемый код.
                                                                                • +1
                                                                                  Вообще-то это гипербола. Или вы имеете ввиду быстроту написания кода, а не его выполнение?

                                                                                  Очень просто: идеальная читаемость НЕ делает программу медленной.
                                                                                  Ваш тезис не преувеличение (гипербола), а прямая дезинформация


                                                                                  Самые быстрые куски кода пишутся на asm иногда, или похоже на asm, без объектов, вызовов функций, лямбд и т.д.

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

                                                                                  • 0
                                                                                    Вам не кажется что тут взаимоисключающие утверждения?

                                                                                    Идеальная читаемость НЕ делает программу медленной.

                                                                                    Код с низкоуровневыми ручными оптимизациями читается плохо.
                                                                                    • +1

                                                                                      Не кажется. Импликация и тождество — разные вещи.

                                                                                      • 0
                                                                                        Вы молодец. Вы очень хорошо объяснили.
                                                                                        Я теперь чувствую себя действительно обиженным, что этого не понимал.
                                                                                    • –1
                                                                                      Комментарии нельзя редактировать, так-что:
                                                                                      Это иллюстрирует совершенно другое явление: код с низкоуровневыми ручными оптимизациями читается плохо.

                                                                                      Я понял в чем мы не сходимся. Для меня это все, это один (единый) код. С которым надо работать, который подлежит и оптимизации, и редактированию для хорошей читаемости.
                                                                                      • 0
                                                                                        Для меня это все, это один (единый) код. С которым надо работать, который подлежит и оптимизации, и редактированию для хорошей читаемости

                                                                                        Нет. Читаемость нужна априори и везде.
                                                                                        Низкоуровневая ручная оптимизация нужна там и только там, где имеющиеся программные инструменты не позволяют добиться требуемой производительности.
                                                                                        "Преждевременная оптимизация — корень всех зол в программировании" (с)

                                                                              • +1
                                                                                особенно если где-то в конце цепочки else if идёт запрос к базе, а в предыдущих его нет

                                                                                Запрос внутри хрен знает какого условия else-if? Даже страшно подумать, что такой говнокодище может достаться для поддержки
                                                                                • 0

                                                                                  Не вижу говнокода в проверке вида if (someValue || (someValue = requestFromDatabase("SomeValue"))) если она встречается в коде в единственном числе.

                                                                              • 0
                                                                                Читаемость кода и производительность (мне кажется) в большинстве случаев находятся по разные стороны.

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

                                                                              • +1
                                                                                И эти люди нам пытаются продать анализатор кода.
                                                                                Эти пока не пытаются.
                                                                                • +1

                                                                                  Учитывая, что в статье единственная ссылка ведёт на вполне определённый анализатор кода, целью её написания явно является product placement.

                                                                                • 0
                                                                                  И эти люди нам пытаются продать анализатор кода.

                                                                                  Нет, это другие люди.
                                                                                  • +1
                                                                                    тогда можно начиная с C++17 If с инициализацией использовать и определять выражения в ней.
                                                                                    • 0

                                                                                      да, это другие люди. Для меня блог pvs-studio является естественным источником кода с багами из-за дефицита code quality для статьи на Хабре. Если подскажете лучше — будет интересно взглянуть. Хотя если за эту статью мне предложат денег (бывает такое, что предлагают денег за статью постфактум?), не откажусь, честное слово :)


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


                                                                                      Вообще, если фанатично следовать этому правилу (хотя я сам так не делаю конечно), всегда можно переписать else if как
                                                                                      else { let firstConditionIsTrue = longCalculatedBoolean(); let hasSecondCondition = secondCondition != null; if (firstConditionIsTrue && hasSecondCondition ) { .... } }
                                                                                      возможно это и правда будет читабельней в каких-то случаях

                                                                                      • 0

                                                                                        блин, не увидел, что с кодом случилось


                                                                                        else { 
                                                                                           let firstConditionIsTrue = longCalculatedBoolean(); 
                                                                                           let hasSecondCondition = secondCondition != null;
                                                                                           if (firstConditionIsTrue && hasSecondCondition ) {
                                                                                         .    ... 
                                                                                           }
                                                                                        }
                                                                                      • 0

                                                                                        Поэтому в таких случаях нужно выносить логику в отдельную функцию.

                                                                                      • +1
                                                                                        Спасибо за статью. Есть пару вопросов и замечаний:
                                                                                        1) последний пример последнее условие !result.length === 0 не очень читаем
                                                                                        2) на сколько плохо сделать метод в моделе model.hasUserWithId? И сразу связанный с ним — id = 0 не допускается?
                                                                                        3) пример хоть и не Ваш, но раз приводите — почему if else везде вместо ПРОСТО if если каждый блок с прерыванием return?
                                                                                        4) что совсем не понятно — это пример с firstName/lastName и accounts — почему не выделить логику в отдельный метод и хорошенько задокументировать, мол, (1) должны быть указаны имя/фамилия, (2) введён пароль и (3) что ещё
                                                                                        • +1
                                                                                          3) Без else, все if'ы будут вычисляться и проверяться каждый раз, а так только до первого совпадения.

                                                                                          Я тоже за соотвествующие методы, вместо таких локальных переменных. Они, конечно, симпатичнее выглядят, чем просто сравнение параметров, но имхо тоже костыли.
                                                                                          • 0
                                                                                            3) Без else, все if'ы будут вычисляться и проверяться каждый раз, а так только до первого совпадения.

                                                                                            Не будут, метод закончится на первом return, все остальные if не сработают.
                                                                                        • 0

                                                                                          А если просто Case или Switch написать?

                                                                                          • +2

                                                                                            А можете написать, как можно первый же пример из статьи преобразовать в адекватный switch?

                                                                                            • 0
                                                                                              Не знаю, на сколько такое работает в оригинальном языке из статьи, но во многих динамических — довольно просто:

                                                                                              switch (tx) {
                                                                                              	case "+%pi":
                                                                                              	case "-%pi":
                                                                                              	case "+Inf":
                                                                                              	case "-Inf":
                                                                                              	case "%inf":
                                                                                              	case "+Nan":
                                                                                              	case "-Nan":
                                                                                              	case "%nan":
                                                                                              		return 4;
                                                                                              		
                                                                                              	case "+%e":
                                                                                              	case "-%e":
                                                                                              	case "%pi":
                                                                                              	case "Inf":
                                                                                              	case "Nan":
                                                                                              		return 3;
                                                                                              }
                                                                                              


                                                                                              С другой стороны, судя по тому отрезку — это просто такой очень кривой способ реализовать strlen для определенных значений.
                                                                                        • +1
                                                                                          static int ParseNumber(const char* tx)
                                                                                          {
                                                                                            static const char** choices = [ "%eps", "-%pi" ];
                                                                                            static const char** noises = [ "%Nan", "Inf" ];
                                                                                          
                                                                                            ....
                                                                                            if (contains(choices, tx) ) {
                                                                                            {
                                                                                                return CHOICES;
                                                                                            }
                                                                                            if (contains(noises, tx) ) {
                                                                                                return NOISES;
                                                                                            }
                                                                                            ....
                                                                                          }
                                                                                          • +1

                                                                                            упс, выше уже предложили аналогичное.

                                                                                          • +1
                                                                                            if ( this.profile.nameFilled() && this.hasAuth() ) {
                                                                                               ....
                                                                                            }
                                                                                            • +1
                                                                                              Я во время код-ревью также прошу выносить составные выражения из блока if в переменную и давать ей чёткое и ясное имя. Всегда говорю своим коллегам, что код должен читаться, как книжка.
                                                                                              • +1
                                                                                                Ок, согласен. А как правильно переписать код из первого листинга?
                                                                                              • +2
                                                                                                Этот код — прсто треш. Проверку на токены нужно делать токенизаторами, а вовсе не strncmp. Это к тому же еще и работает быстре, а заодно и ошибок будет меньше — потому что все токены (последовательности) будут в одну колонку по вертикали — легко ее отсортировать по порядку и увидеть косяки.
                                                                                                Пример неплохого токенизатора — re2c :)
                                                                                                Почему токенизатор работает быстрее — посмотрите, в вашей исходный код — там мало лидирующих символов — это плюс минус, процент, I и N, т.е. всего пять. Вы можете начать проверку первого символа во входном потоке с этих пяти символов и ветвиться дальше, если есть совпадение. Если писать такое ручками, то ветвление будет выглядеть довольно замысловато, и его будет сложно обслуживать, например сложно будет добавить новую последовательность. К счаcтью есть токенизатор, который сделает это всё за вас.
                                                                                                • 0

                                                                                                  согласен, но пост про code quality, а не про common sense :)

                                                                                                • +1

                                                                                                  Написано всё верно, но вместо переменных нужно использовать функции или методы.

                                                                                                  • +1

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


                                                                                                    Почему-то автор не взял пример из начала статьи и не отрефракторил его с элементами бизнес-логики.
                                                                                                    Можно, конечно, вынести содержимое условия в функцию
                                                                                                    elseif ( strlen(tx) >= 3 && checkStrLen3(tx)) {}, убрав содержимое в функцию "как есть", но тогда будет хороший if, плохой return.
                                                                                                    Можно пойти дальше, заменив strncmp(tx, "+%e", 3) == 0 на функцию isE( tx ), но условие все равно будет из кучи функций


                                                                                                    return isE(tx) || isPi(tx) || isNan(tx) || isInf(tx) || isPi(tx)

                                                                                                    Стало ли проще найти дубликат? не факт.


                                                                                                    const hasSlack = !!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId);
                                                                                                    const hasSomeLoginCredentials = this.password || hasSlack;

                                                                                                    Норм. Пароля может не быть у десятка человек, а поиск колбэком по serviceAccounts — каждому. Зато читаемо.


                                                                                                    По итогу — в названии статьи про if, по тексту — похвальная попытка закопать кишки бизнес-логики поглубже, по факту — попытка не очень удачная — все model.user.id мясом наружу валяются в переменных строками выше.

                                                                                                    • 0

                                                                                                      у меня в изначальном варианте статьи был вариант рефакторинга из "ниче не понятно". но я не стал публиковать его по трем причинам:


                                                                                                      1. Вы правы и опыта в плюсах у меня немного, последний раз писал активно на них лет восемь назад, а вникать заново в контекст не хотелось
                                                                                                      2. У меня нет понимания big picture продукта и даже контекста этого метода
                                                                                                      3. Я скорее не согласен с таким подходом к парсингу принципиально, и соглашусь с Dmitri-D, что в общем случае нужно использовать токены, но см п. 1 и особенно 2

                                                                                                      Коллбэк в JS коде в данном случае совсем не страшен, это синхронная функция, фильтрующая уже имеющийся массив с небольшим числом элементов.


                                                                                                      В данном случае, как мне кажется, призыв к инкапсуляции не обоснован, так как мы в контексте функции собираемся использовать public-данные другого объекта, и нам решать, валиден он в нашем контексте или нет. Вот если такая проверка не отвечает принципу DRY, то это уже другой разговор.

                                                                                                  • +1
                                                                                                    Полностью согласен с главной мыслью.
                                                                                                    Оптимально — в операторе If должно быть только одно условие. Максимум — два. Если их там больше — целесообразно все их вынести в функцию с осмысленным названием.
                                                                                                    • +1
                                                                                                      Никогда не говори никогда, как говориться.
                                                                                                      Разные бывают ситуации и разные подходы.
                                                                                                      Бывает, что и длинный IF вполне себе понятен и не требует рефакторинга и выноса в переменные или функции.
                                                                                                      Зачем использовать слова «никогда».
                                                                                                      С них обычно и начинаются засады.
                                                                                                      Жизнь гибче и многогранней.
                                                                                                      • 0

                                                                                                        ну это просто мое понимание того, как должно выглядеть название поста, в который хочется зайти и написать комментарий, так-то я пишу, что все не так однозначно конечно :)

                                                                                                      • +2

                                                                                                        Последний пример "когда можно не выносить" убил весь дух статьи… Играем, не играем, рыбу заворачиваем. Надо писать в одном стиле, особенно если в команде уже есть code conventions.

                                                                                                        • 0

                                                                                                          это все сведется с бесконечным const isError = ....; if(isError) { .... }. Никакой дополнительной информации вы из этого не получите, условия просты как две копейки — зачем такой формализм?

                                                                                                          • +1

                                                                                                            Ну и писать константу для обычной проверки на null или undefined я тоже считаю избыточным. Не то что я прямо предлагаю экономить память на указатели, но меня учили по старому: есть данные — юзай.

                                                                                                        • –3
                                                                                                          Не так страшно длинное условие в этом самом conditional statement'e
                                                                                                          как точка выхода из функции посреди функции.

                                                                                                          «Так верстают только ...»
                                                                                                          • +1
                                                                                                            Вы из тех, кто считает, что точка выхода должна быть всего одна?
                                                                                                            • 0
                                                                                                              Вы-таки предпочитаете делить людей на группы «кто за» и «кто против»?
                                                                                                              • 0
                                                                                                                Ваша категоричность бескомпромиссна. Потому и спросил.
                                                                                                                А люди и без моей помощи себя отлично делят на группы.
                                                                                                                • 0
                                                                                                                  Мнение каждого человека категорично и безкомпромисно в первом приближении,
                                                                                                                  если это действительно его мнение,
                                                                                                                  иначе — это заискивание перед (собеседником|окружающими).
                                                                                                                  • 0

                                                                                                                    А варианты "не знаю" и "не уверен"- Вы в принципе не рассматриваете?
                                                                                                                    Бескомпромиссность мнения по вопросу, которого человек не знает — признак самодурства.

                                                                                                          • 0
                                                                                                            strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0

                                                                                                            надо реализовать до логического конца: объявить массив подстрок и определить функцию, проверяющую вхождения в строку. типа IsContains(tx, длина tx, указатель на массив подстрок, длина массива подстрок).
                                                                                                            тогда всё будет проще.
                                                                                                            • +1
                                                                                                              С простыми примерами всё понятно, а как лучше переписать вот это условие в GCC?
                                                                                                              • 0
                                                                                                                Вынести в отдельную процедуру, которая возвращает true или false. И потом разбить на множество ретурнов. Маленькие независимые блоки читаются значительно легче одного огромного. Ну и участки, которые вычисляются несколько раз — записать в переменную и использовать уже ее.

                                                                                                                На самом деле если присмотреться, то это не один огромный атомарный блок, там есть множество независимых условий. Вот допустим блок не должен выполняться, если «in == 0». Почему бы его и не выделить в отдельную логическую группу?

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

                                                                                                                bool core_condition() {
                                                                                                                    // мы должны тут выйти, потому-что ...
                                                                                                                    if (in == 0) {
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                
                                                                                                                    // а тута...
                                                                                                                    if (GET_CODE (in) != SUBREG) [
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                
                                                                                                                    // If subreg_lowpart_p is false, we cannot reload just the
                                                                                                                	// inside since we might end up with the wrong register class
                                                                                                                	// But if it is inside a STRICT_LOW_PART, we have
                                                                                                                    // no choice, so we hope we do get the right register class there
                                                                                                                    if (subreg_lowpart_p (in) == false && strict_low == false) {
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                	
                                                                                                                    var subreg = SUBREG_REG(in);
                                                                                                                    var subreg_mode = GET_MODE(subreg);
                                                                                                                
                                                                                                                    // bla-bla
                                                                                                                #ifdef CANNOT_CHANGE_MODE_CLASS
                                                                                                                    if (CANNOT_CHANGE_MODE_CLASS(subreg_mode, inmode, rclass)) {
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                #endif
                                                                                                                    
                                                                                                                    // bla-bla
                                                                                                                    if (!contains_allocatable_reg_of_mode[rclass][subreg_mode]) {
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                    
                                                                                                                    // bla-bla
                                                                                                                    if (CONSTANT_P(subreg) || GET_CODE(subreg) == PLUS) {
                                                                                                                        return true;
                                                                                                                    }
                                                                                                                    
                                                                                                                    // bla-bla
                                                                                                                    if (strict_low) {
                                                                                                                        return true;
                                                                                                                    }
                                                                                                                
                                                                                                                        // .......
                                                                                                                        // .......
                                                                                                                        // .......
                                                                                                                    
                                                                                                                #ifdef CANNOT_CHANGE_MODE_CLASS
                                                                                                                    if (!REG_P(subreg) || REGNO(subreg) >= FIRST_PSEUDO_REGISTER) {
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                    
                                                                                                                    if (!REG_CANNOT_CHANGE_MODE_P(REGNO(subreg), subreg_mode, inmode)) {
                                                                                                                        return false;
                                                                                                                    }
                                                                                                                #endif
                                                                                                                
                                                                                                                    return false;
                                                                                                                }
                                                                                                                

                                                                                                              • 0
                                                                                                                по теме статьи:
                                                                                                                да, когда читал помню Макконнелла то мне казалось — хачем такие длинные имена обычным переменным? проще ж всякие там i, k — их и набирать быстрее. теперь это конечно вспоминается с улыбкой. как и то, что Си мне нравился больше Паскаля уже хотя бы потому, что писать { } быстрее чем begin и end. но когда begin пишешь с той же скоростью, что и скобку(а по правде — не пишешь вообще, из-за снипетов), а длинное имя переменной кажется не таким уж длинным — то все меняется. хороший код зачастую читается как русский язык и понятен с ходу. оптимизированный — лучше детально комментировать.

                                                                                                                в своей практике(я думаю что так у любого программиста) — появилось некое такое спинно-мозговое чувство говнокода. т.е. изза человеческого фактора(усталость, позднее время суток) — понимаешь что так писать нельзя, но результат увидеть хочешь. в таких случаях как я советую поступать(сам уже давно так делаю):

                                                                                                                if ....  // govnokod
                                                                                                                {
                                                                                                                
                                                                                                                }
                                                                                                                
                                                                                                                


                                                                                                                что это дает? во первых помогает бороться со своим ЧСВ. во вторых — облегчает поиск говнокода когда проект в заработал как надо. в третьих — если вы вернулись к проекту через пару лет — то вы видя этот код будете сразу знать, что во время написания вы считали ЭТО — говнокодом. Это относиться к синтаксису, или можно так сказать — к структурным ошибкам кода. как верно было замечено выше — условие никогда не появилось бы в корявом виде при использовании токенов(т.е. изменении структуры кода, при которой код меняется настолько, что говнокод исчезает сам собой). видя через пару лет подсказку оставленную ранее вы с легкостью понимаете, что тут еще есть где поработать над структурой, чтобы СИЕ — просто исчезло.

                                                                                                                более мягкий вариант совета 1:

                                                                                                                if ....  // can be optimized by using tokens
                                                                                                                {
                                                                                                                
                                                                                                                }
                                                                                                                
                                                                                                                


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

                                                                                                                и чуть-чуть не по теме:
                                                                                                                совет 3. честно — надеюсь его прочтут разработчики компиляторов, т.к. даже скорее пожелание а не совет. в плане фигурных скобок в Си и begin,end в Паскале — я пришел к тому что в идеале не писать ни того не другого. В этом плане хорош(блестательно, выдающийся) — Питон. во первых это привносит реальную дисциплину в форматирование кода и убирает все разногласия по этому поводу. во вторых экономит место по вертикали. в третьих экономит время на набор и форматирование кода. в четвертых избавляет ИДЕ от необходимости прорисовки всяческих вертикальных полосок. наличие же начал и концов блоков порождает вечный дискус о том что было в начале — курица или яйцо, занимая время, котрое могло быть потрачено на написание полезного кода.

                                                                                                                • –1
                                                                                                                  Эффективно уменьшить глубину бинарного дерева ( де факто сделать читабельным громадный блок кода состоящий из условий и циклов) позволит автоматный подход. «Паттерн State» для С++. В случае чистого Си, это основательно забытый «Daff-Device». Современный компилятор, в отличие от компиляторов 90-х, великолепно оптимизирует автоматный подход. Не забываем проверять результирующий ASM с включенной оптимизацией. Сейчас это удобнее всего делать на godbolt.org с флагом для GCC "-O3". С другой стороны, как подсказывает опыт, такие громадные простыни условий создаются не вручную. Это скорее всего результат работы генератора кода. Например Yacc, Бизон, Antlr… И их результирующий код вообще не предназначен для правки «вручную». Скорее всего BNF и скрипт шага прекомпиляции были приложены к проекту, но какой-то «умник» их просто снёс.
                                                                                                                  А уже какой-то другой «умник» таки попытался править результат генерации вручную.
                                                                                                                  • 0
                                                                                                                    Никогда не давайте рекомендаций, начинающихся словами «никогда не...».

                                                                                                                    К кодеру голова приделана не чтобы в неё есть, а чтобы думать, как в какой ситуации писать.

                                                                                                                    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.