Code Review case 1

    Я работал в компании, в которой отсутствует практика code review. Для самосовершенствования и расширения кругозора я бы хотел получить немного конструктивной критики.

    Сейчас предлагаю разобрать повторяющийся случай с обилием ветвлений.

    Задание


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

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

    Если папка назначения находится на том же разделе, что и файл, то переместить файл. Невозможно переместить: нет прав на запись; полный путь назначения окажется слишком длинным, в папке уже есть файл с тем же именем (вызвать диалог); файл системный и его нельзя удалить; уже есть файл с таким имененем (вызвать диалог на перезапись файла, если пользователь согласен, то перезаписать).

    Если пользователь перенес файл в другой окно, но оно имеет тот же путь, то создать копию файла (Добавить к имени « копия #», где # – наименьшее положительное число, делающее файл уникальным). Невозможно создать копию: нет прав на запись; полный путь слишком длинный; не хватает свободного места.

    Если пользователь переносил правой кнопкой, то вызвать диалог для выбора действия (скопировать/переместить/создать ярлык/создать копию).

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

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

    Решение для обсуждения


    Предлагаю своё спорное решение на Java, в котором я достиг второго наибольшего уровня вложенности if:

    public static void dispatchFileDropping(
            FileDragNDropEvent event
    ) {
        
        //------------------------------------------------------
        // Исходные предикаты 
        //  (имена даны в стиле математической логики).
        
        boolean A = isTargetPlaceIsDirectoryWindow(event);
        boolean B = isTargetDirEqualsSourceDir(event);
        boolean C = isTargetVolumeEqualsSourceVolume(event);
        boolean D = isMouseRightButtonUsed(event);
        boolean E = isSystemFileDroped(event);
        boolean F = isTargetVolumeHasFreeSpace(event);
        boolean G = isTargetDirWritable(event);
        boolean H = isSourceDirCleanable(event);
        boolean I = isFileNameOkForTarget(event);
        boolean J = isNewFileFullPathOkForTargetLimit(event);
        boolean K = isTargetDirHasSameNamedFile(event);
        boolean L = isTargetDirSameNamedFileIsWritable(event);
        
        Actions userChoise 
            = (A & D) ? askUserForAction(event) : null;
        if (userChoise == Actions.CANCEL) return;
        
        boolean M = (userChoise == Actions.COPY);
        boolean N = (userChoise == Actions.CLONE);
        boolean O = (userChoise == Actions.MOVE);
        boolean P = (userChoise == Actions.LINK);
        
        
        //------------------------------------------------------
        // С каким случаем имеем сейчас дело.
        
        boolean copyCase = (M & !K) | (A & !B & !C & !D & !K);
        boolean copyRewriteCase 
            = (M & K) | (A & !B & !C & !D & K);
        boolean cloneCase = N | (A & B & !D);
        boolean moveCase = (O & !K) | (A & !B & C & !D & !K);
        boolean moveRewriteCase = (O & K) | (A & !B & C & !D & K);
        boolean createLinkCase = P;
        
        
        //------------------------------------------------------
        // Пользователь может отказаться от перезаписи 
        //  существующего файла.
        
        if (copyRewriteCase | moveRewriteCase) {
            if (askUserWantToRewrite() == Answers.NO) return;
        }
        
        
        //------------------------------------------------------
        // Вычисляем возможность для каждого случая.
        
        boolean isPossibleToCopy = F & G & I & J;
        boolean isPossibleToCopyRewrite = isPossibleToCopy & L;
        boolean isPossibleToClone = isPossibleToCopy;
        boolean isPossibleToMove = isPossibleToCopy & !E & H;
        boolean isPossibleToMoveRewrite = isPossibleToMove & L;
        boolean isPossibleToLink = isPossibleToCopy & !K;
        
        
        //------------------------------------------------------
        // Либо выполняем операцию, либо объясняем, 
        //  почему не выполнили ее.
        
        String errorMessage = "";
        if (copyCase & !isPossibleToCopy) {
            errorMessage = "Невозможно скопировать файл.";
        } else if (copyRewriteCase & !isPossibleToCopyRewrite) {
            errorMessage = "Невозможно перезаписать файл.";
        } else if (cloneCase & !isPossibleToClone) {
            errorMessage = "Невозможно создать копию.";
        } else if (moveCase & !isPossibleToMove) {
            errorMessage = "Невозможно переместить файл.";
        } else if (moveRewriteCase & !isPossibleToMoveRewrite) {
            errorMessage 
                = "Невозможно переместить файл с заменой.";
        } else if (createLinkCase & !isPossibleToLink) {
            errorMessage = "Невозможно создать ссылку.";
        }
        
        String reasons = " Причины: \n";
        if (!F) { 
            reasons += "-- закончилось место на диске \n";
        }
        if (!G) {
            reasons 
            += "-- нет прав на запись в папке назначения \n";
        }
        if (!I) {
            reasons 
            += "-- имя файла содержит недопустимые символы \n";
        }
        if (!J) {
            reasons
            += "-- полный путь нового файла превышает предел \n";
        }
        if (moveCase | moveRewriteCase) {
            if (E) {
                reasons 
                += "-- нельзя удалить системный файл \n";
            }
            if (!H) {
                reasons 
                += "-- нет прав на удаление в папке \n";
            }
        } else if (copyRewriteCase | moveRewriteCase) {
            if (!L) {
                reasons 
                += "-- нет прав на изменение файла \n";
            }
        } else if (createLinkCase) {
            if (K) {
                reasons 
                += "-- файл с таким именем уже существует \n";
            }
        } 
        
        if (errorMessage.isEmpty()) {
            if (copyCase) copy(event);
            if (copyRewriteCase) copyRewrite(event);
            if (cloneCase) clone(event);
            if (moveCase) move(event);
            if (moveRewriteCase) moveRewrite(event);
            if (createLinkCase) createLink(event);
        } else {
            showMessage(errorMessage + reasons);
        }
    }
    
    Поделиться публикацией
    Комментарии 30
      +13
      А теперь, не подглядывая в код, что значит "(M & !K) | (A & !B & !C & !D)"?
        –5
        Как предлагаете записать иначе? Условие может быть очень длинным, например таким: !(A & H) & (K | !D | (F & D) | !(E & !G)) & Z & !Y
        При этом каждая буква может расшифровываться как очень длинное предложение, если дать нормальное имя, то оно будет состоять из нескольких слов, пяти, шести. Так хотя бы видны дизъюнкции, конъюнкции, отрицания и скобки. Расшифровка букв всегда рядом.
          +2
          Расшифровка — это костыль, хорошему программисту следует сделать все, чтобы его код можно было прочитать без дополнительных расшифровок. Пусть он будет длиннее на порядок, но читабельней, да и с длинными предложениями никто не мешает поступать переносом каждого условия с нормальным форматированием

        +3

        По именованию переменных жесть конечно. Такое чувство как будто в один момент стало лень придумывать названия.


        boolean isPossibleToCopyRewrite = isPossibleToCopy & L;

          +1
          Нужно определить или использовать существующую структуру для валидации, метод должен проверять условия и возвращать экземпляр структуры с результатами валидации. Никакой конкатенации строк, коллекции. Метод валидации не должен показывать никаких окон.
            +1

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


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


            Было
                boolean copyCase = (М & !К) | (А & !B & !C & !D & !K);
                boolean copyRewriteCase 
                    = (M & K) | (А & !В & !C & !D & К);
                boolean cloneCase = N | (A & B & !D);
                boolean moveCase = (O & !K) | (А & !B & C & !D & !К);
                boolean moveRewriteCase = (O & К) | (A & !В & C & !D & K);

            Получится вот такое:


            Переделка №1
                boolean copyCase = false;
                boolean copyRewriteCase = false;
                boolean cloneCase = false;
                boolean moveCase = false;
                boolean moveRewriteCase = false;
                boolean createLinkCase = false;
            
                if (A & D){
                    Actions userChoice = askUserForAction(event);
            
                    if (userChoise == Actions.CANCEL) return;
            
                    boolean M = (userChoise == Actions.COPY);
                    boolean N = (userChoise == Actions.CLONE);
                    boolean O = (userChoise == Actions.MOVE);
                    boolean P = (userChoise == Actions.LINK);
            
                    copyCase = М & !K;
                    copyRewriteCase = M & К;
                    cloneCase = N;
                    moveCase = O & !К;
                    moveRewriteCase = O & K;
                    createLinkCase = P;
                } else {
                    copyCase = (A & !В & !C & !D & !K);
                    copyRewriteCase = (А & !В & !C & !D & К);
                    cloneCase = (А & B & !D);
                    moveCase = (A & !B & C & !D & !К);
                    moveRewriteCase = (А & !B & C & !D & K);
                }

            Далее можно заменить M,N,O,P на выражения вроде userChoise == Actions.COPY.


            Становится видно, что:


            • вообще любой из кейсов требует истинности A. Кажется можно проверить эту переменную где-нибудь в начале и больше ничего не проверять.
            • Любой из кейсов в блоке elseтребует ложности D. Поскольку выполнение в else, можно убрать предикат !D.

            В блоке else останется вот такое выражение


            Переделка №2
            else {
                copyCase = !В & !C & !K;
                copyRewriteCase = (!B & !C & К);
                cloneCase = (В);
                moveCase = (!B & C & !К);
                moveRewriteCase = (!B & C & K);
            }

            которое можно развернуть в дерево if'ов


            Переделка №3
            else if (B) {
                cloneCase = true;
            } else {
                if (C){
                    moveRewriteCase = K;
                    moveCase = !К;
                }
                copyCase = !В & !C & !K;
                copyRewriteCase = (!B & !C & К);
            }

            copyCase и copyRewriteCase можно написать по аналогии с moveCase и moveRewriteCase.


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


            Чтобы было не скучно, я поменял в своем комментарии кое-где в коде английские буквы на аналогично выглядящие русские.

              –1

              Вы пришли к тому, от чего я убежал. Раньше я делал либо 10-уровневые деревья, либо сочетания 5-уровневых деревьев с вынесенными отдельными поддеревьями с досрочным return. Во время изменения требований я путался и делал ошибки, так как приходилось держать в уме ситуации досрочных выходов и при необходимости возвращать вынесенные отдельные поддеревья в единое общее дерево. Каждый раз приходиться пересматривать все ветки и все вынесенные поддеревья, это было утомительно. Ведь когда-нибудь появится действие на предикат! А с сочетанием с другим предикатом и т.д.

                –1
                Например, заказчик добавляет:
                Пользователь сначала перенес файл, что вызвало окно с прогрессом копирования. Далее пользователь переносит другой файл, но опускает не в окне папки, а в окне с монитором прогресса копирования. В этом случае нужно проверить, что можно скопировать файл, что данный файл в данный момент не копируется, после чего поставить файл в очередь на копирование.


                Изменения в коде плоские и линейные:
                    //------------------------------------------------------
                    // Исходные предикаты 
                    // ...
                    boolean Z = isTargetPlaceIsCopyingMonitor(event);
                    boolean Y = isQueueContainsFile(event);
                
                    boolean copyQueueCase = Z & !K;
                    boolean copyQueueRewriteCase = Z & K;
                
                    // ...
                    if (errorMessage.isEmpty()) {
                        // ...
                        if (copyQueueCase) addToCopyQueue(event);
                        if (copyQueueRewriteCase) addToCopyQueueRewrite(event);
                    } else {
                        showMessage(errorMessage + reasons);
                    }
                


                Проверку на невозможность я не сделал, но там тоже изменения плоские и линейные
                  +4

                  Лучше бы не убегали. В предикатах вроде (M & !K) | (A & !B & !C & !D & !K); запутаться так же просто, как и в "лесу" из if'ов.


                  Если предлагаемый вариант не подходит, то попробуйте каждую ситуацию вынести в свой класс с методами "canBeExecuted(event)" и "execute(event)". Экземпляры ситуаций положите в коллекцию и при каждом событии вызывайте execute первой стратегии, у которой canBeExecuted вернет true. Внутри execute можно будет написать соответствующие обработчики ошибок.

                    0
                    Наверное так будет лучше всего
                0
                Просится выделение отдельного метода валидации, который кроме проверки ничего не делает. Еще хочется более осмысленного названия переменных, хотя бы мнемоники, т.е. вместо А — TPIDW, ну или чего-то получше. Также повторяющиеся куски ((A & !B & C & !D & K)) вынести и дать осмысленные названия.
                  –1
                  Я бы завернул такой PR.
                  1. В логику булевых операций вмешан запрос подтверждения от пользователя askUserWantToRewrite(). Это увеличивает связность и без того сложного метода
                  2. Результат метода — действие с файлом или показ сообщения.

                  По-хорошему нужно выделить отдельный метод на проверку всех параметров. Метод будет возвращать признак операции, можно ли её совершить, признак необходимости подтверждения пользователя и причину, почему выполнить нельзя.
                  Результат метода передаётся в соответствующий обработчик действия, которых должно быть столько, сколько есть возможных действий.

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

                  P. S. Не вижу проблемы в названиях переменных. Это тот исключительный случай, когда названиями переменных лучше пожертвовать для понимания сложных булевых операций.
                    +1
                    «другая папка находится на другом разделе, то проверить возможность копирования. Если скопировать можно, то скопировать. Иначе выдать сообщение, что нельзя скопировать.»

                    Вот не надо так делать:
                    А) лишняя проверка, всё равно при копировании будет сделано тоже самое системой.
                    Б) бессмысленная проверка, перед копированием, но после проверки возможность может исчезнуть (кончилось место, например)
                    Поэтому надо копировать и разбирать код возврата. Реймон Чен (ЕМНИП) об этом говорил.
                      +1
                      Перед тем, как менять названия переменных и наводить красоту, было бы хорошо поработать над тем, чтобы отделить UI от логики. В функции сначала вычисляются предикаты, а потом 1 или 2 раза запрашиваются действия пользователя: askUserForAction(event) и askUserWantToRewrite(). Пользователь может не отвечать довольно долго. За то время, пока он решит ответить, ситуация в окружении может измениться значительно: может закончиться место, кто-нибудь другой может добавить файл с таким же названием в директорию назначения, директория назначения может быть вообще удалена. Да и как-то это неправильно смешивать бизнес-правила с сценарием UI.
                        +1

                        Будьте проще. Нет, намного проще.


                        1. Сначала надо определить совершаемое действие, и только его: либо это перенос, либо это копирование, либо дублирование (дублирование — это, в данном случае, копирование файла в файл с именем "… копия x"). Если перенос был левой кнопкой, действие определяется автоматически, если правой — по выбору пользователя (а "автоматическое" используется как дефолт).
                        2. Запустить соответствующую операцию, если произошла одна из перечисленных ошибок, разбираем ее.
                        3. Ошибки делятся на те, где от пользователя нужен дальнейший ввод, и все остальные (retry-cancel мы за ввод не считаем, это общая стратегия); у дублирования ошибок-со-вводом не бывает, у остальных операций это ситуация "файл с таким именем уже существует".
                        4. Если пользователь задал новое имя, запускаем ту же операцию, но с явным указанием целевого имени, и возвращаемся на шаг 2.

                        PS Если речь идет о Java или любом другом ОО-языке, каждая операция — это объект, а повторного использования кода можно добиться наследованием или композицией, в зависимости от ваших архитектурных предпочтений и языка.

                          0
                          Я не знаю насколько в java реализуется такой подход, но мне кажется что наиболее читаемым было бы использование либо pattern matchin либо промисов, либо и того и другого.
                          Пример достаточно объемный чтобы его переписать полностью, но я бы с удовольсвие видело код вида следующего псевдокода

                          switch (action, someBoolFlag, secondBoolFlaga) {
                          case Actions.COPY, namedBoolFlagA, namedBoolFlagB: action.IfTargetDirNotEqualsSourceDir().IfTargetVolumeHasFreeSpace.Then(
                          askUserWantToRewrite().Then(action.Execute())
                          ).Error(.....)
                          }


                          Error должен получать имя и некий объект от того промиса который его вызвал.
                          И тогда это было бы читаемо и понятно. Понятно что можно еще разделить на отдельные подпросимы и использховать их не один раз.

                          Заодно это бы решило вопросы с вещами вида сполнения askUserWantToRewrite и askUserForAction — так как промисы бы выполнялись в отдельном «потоке», то это бы не блокировало выполнение программы как таковой.
                            +2
                            Довольно типичный пример многоуровневого косяка. Начинается всё с бизнес-логики (помесь астрологии и херомантии — когда луна в сатурне делаем то-то). Одно из главных правил разработки пользовательского интерфейса — консистентность. Перетаскивание файла должно приводить только к одному действию вне зависимости от гороскопа пользователя. В Windows это слегка нарушено (в пределах одного диска это перемещение, разные диски — копия). В зависимости от модификаторов (нажатая кнопка контрол) это может быть копирование или (нажатая кнопка альт) создание ярлыка или (перетаскивание правой кнопкой мыши) контекстное меню. Имея четкие правила изначально, вся логика укладывается в простейший try-catch.

                            Идем дальше, как уже справедливо заметили выше, ранние проверки в лучшем случае бесполезны а по факту даже вредны (так как ухудшают читаемость и плодят немеряную сложность на ровном месте). Меньшая часть логики должна сидеть в блоке обработки ошибок, большая часть должна быть просто удалена. Никого не волнует причина по которой не может быть выполнено копирование (дисковое пространство, права доступа или водолей в овне).
                              0
                              > Никого не волнует причина по которой не может быть выполнено копирование

                              Смелое заявление.
                                –1
                                Я скажу даже смелее. Вы никогда не можете быть уверены что верно идентифицировали причину, по которой копирование невозможно. К примеру вы поймали IOException с сообщением «There is not enough space on the disk». Нормально, на диске не хватает места. Как привязаться — ну очевидно по строке. А потом на другой машине ловите «No space left on device». Или «На диске не хватает места» или еще что-нибудь (потому что сообщения OS-зависимые). Или пытаетесь создать файл с определенным именем, а такой файл уже есть, просто у вас даже на чтение недостаточно прав чтобы его увидеть. Или просто ситуация когда не хватает прав — ничего вы этого понять в программе не можете, потому что все что вы имеете это IOException. Вот и получается что правильная последовательность действий одна — показать сообщение ошибки пользователю и дать на выбор «Повторить» или «Отменить».
                                  0
                                  Я всё же полагаю, что «никого не волнует причина» и «не всегда возможно точно определить причину» — это две большие разницы. Когда лично я копирую файлы и не могу их скопировать (да и вообще, совершаю какое-либо действие и не добиваюсь результата) — меня причина интересует всегда. Думаю, как и большинство здесь собравшихся.
                              +3
                              Есть чуть более специализированные ресурсы для этого, например codereview.stackexchange.com
                              • НЛО прилетело и опубликовало эту надпись здесь
                                  0
                                  Здесь проблема не в тестировании и тестируемости. Хотя и в этом тоже, так как для того, чтобы протестировать такую смесь UI, бизнес-правил и состояния окружения, надо написать кучу моков, выделить кучу интерфейсов и т.д. И получится вместо одного непонятного метода куча непонятных методов и классов.
                                  Проблема в читаемости. Код пишется один раз, правится несколько раз, а читается иногда десятки и сотни раз. Прочитать написанное очень сложно: приходится постоянно прыгать в начало или конец метода, приходится постоянно проверять, что означают непонятные переменные, а также прослеживать все цепочки до конца кода. Возьмём, например, строку: «Actions userChoise = (A & D)? askUserForAction(event): null;». По каким причинам userAction может стать null? Потому что не (A & D). А что такое A? А что такое D? Возвращаемся в самое начало, чтобы выяснить. А может ли метод askUserForAction вернуть null, при том что (A &D)? Лезем смотреть реализацию askUserForAction. А если userAction реально станет null? Случится ли что-нибудь плохое? Надо проследить всё до самого конца, разобраться в каждой проверке, чтобы понять, что ничего страшного не произойдёт, хотя можно было бы сразу выйти.
                                  • НЛО прилетело и опубликовало эту надпись здесь
                                      0
                                      А так конечно хорошо быть эльфом в стране эльфов и всегда писать идеальный код, но задач много, времени мало и иногда чуток говнокода, здоровый цинизм и юнит-тесты позволяют сворачивать горы гораздо меньшими усилиями.


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

                                          К сожалению, не совсем, ведь код содержит и логику проверки, и взаимодействие с пользователем, и взаимодействие с файловой системой.
                                          Если разнести всё по отдельным классам, то и покрывать тестами будет намного проще.
                                            0
                                            Прежде чем давать совет «покрывать всё тестами», было бы неплохо спросить себя — а стал бы я покрывать тестами такой код? И что-то мне кажется что в этом случае ответ будет «нет» — время на заведомую ерунду тратить жалко.
                                    +1
                                    Этот подход для новичка может показаться нормальным, но он имеет очень много «косяков», и не подходит для больших проектов. Давай по порядку:
                                    — Имена переменных. Выше обсуждалось, код должен быть читабельным без комментариев.

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

                                    — Много переменных. Состояния мыши в можно объединить в структуру и передавать её между методами. Можно будет написать проверку не так:
                                    boolean copyCase = (M & !K) | (A & !B & !C & !D & !K);

                                    а, хотя бы так:
                                    boolean copyCase = isCopyCase(mouseState);

                                    где в isCopyCase я бы скомбинровал проверку правильным образом.

                                    — Код будет редактироваться другими членами команды, и чтобы оставить его «читабельным», они должны писать так-же неправильно. И скорее всего они не будут следовать твоим внутренним правилам, и через 3-4 сторонние правки вряд-ли сам поймешь, что происходит в коде.

                                    — Для постройки строк оптимальнее использовать StringBuilder.

                                    — Правильное разделение кода на пакеты, классы, методы дает возможность покрывать код тестами. Код например можно разделить на блоки: «вычитка состояния», «проверка», «выполнения действия». И нет ни одной причины писать всё это в одном вместе.

                                    В общем, есть интересная книжка — «Боб Мартин — Чистый код», она перевернёт твой взгляд, как однажды перевернула и мой. Советую почитать.
                                      0

                                      Подключите к сборке checkstyle & pmd и попробуйте некоторое время обходиться без suppress-ов или отключения правил.

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

                                      Самое читаемое