AMURWOLF Jul 23 2018 at 07:02

Code Review case 1

4 min

4.5K

Programming*

Comments 30

Valle Jul 23 2018 at 07:16

А теперь, не подглядывая в код, что значит "(M & !K) | (A & !B & !C & !D)"?

AMURWOLF Jul 23 2018 at 07:26

Как предлагаете записать иначе? Условие может быть очень длинным, например таким: !(A & H) & (K | !D | (F & D) | !(E & !G)) & Z & !Y
При этом каждая буква может расшифровываться как очень длинное предложение, если дать нормальное имя, то оно будет состоять из нескольких слов, пяти, шести. Так хотя бы видны дизъюнкции, конъюнкции, отрицания и скобки. Расшифровка букв всегда рядом.

peresada Jul 23 2018 at 07:36

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

The_Pro Jul 23 2018 at 07:32

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

boolean isPossibleToCopyRewrite = isPossibleToCopy & L;

posledam Jul 23 2018 at 08:17

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

akryukov Jul 23 2018 at 08:58

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

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

Было

    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.

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

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

AMURWOLF Jul 23 2018 at 09:23

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

AMURWOLF Jul 23 2018 at 09:42

Например, заказчик добавляет:

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

Изменения в коде плоские и линейные:

    //------------------------------------------------------
    // Исходные предикаты 
    // ...
    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);
    }

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

akryukov Jul 23 2018 at 09:45

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

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

AMURWOLF Jul 23 2018 at 09:49

Наверное так будет лучше всего

maxzh83 Jul 23 2018 at 09:39

Просится выделение отдельного метода валидации, который кроме проверки ничего не делает. Еще хочется более осмысленного названия переменных, хотя бы мнемоники, т.е. вместо А — TPIDW, ну или чего-то получше. Также повторяющиеся куски ((A & !B & C & !D & K)) вынести и дать осмысленные названия.

UFO landed and left these words here

NickViz Jul 23 2018 at 10:22

«другая папка находится на другом разделе, то проверить возможность копирования. Если скопировать можно, то скопировать. Иначе выдать сообщение, что нельзя скопировать.»

Вот не надо так делать:
А) лишняя проверка, всё равно при копировании будет сделано тоже самое системой.
Б) бессмысленная проверка, перед копированием, но после проверки возможность может исчезнуть (кончилось место, например)
Поэтому надо копировать и разбирать код возврата. Реймон Чен (ЕМНИП) об этом говорил.

AndreyGaskov Jul 23 2018 at 10:51

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

lair Jul 23 2018 at 11:37

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

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

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

YoungSkipper Jul 23 2018 at 11:49

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

switch (action, someBoolFlag, secondBoolFlaga) {

 case Actions.COPY, namedBoolFlagA, namedBoolFlagB: action.IfTargetDirNotEqualsSourceDir().IfTargetVolumeHasFreeSpace.Then(

 askUserWantToRewrite().Then(action.Execute())

).Error(.....)

}

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

Заодно это бы решило вопросы с вещами вида сполнения askUserWantToRewrite и askUserForAction — так как промисы бы выполнялись в отдельном «потоке», то это бы не блокировало выполнение программы как таковой.

TerraV Jul 23 2018 at 12:07

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

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

wormball Jul 23 2018 at 19:09

> Никого не волнует причина по которой не может быть выполнено копирование

Смелое заявление.

TerraV Jul 23 2018 at 22:22

Я скажу даже смелее. Вы никогда не можете быть уверены что верно идентифицировали причину, по которой копирование невозможно. К примеру вы поймали IOException с сообщением «There is not enough space on the disk». Нормально, на диске не хватает места. Как привязаться — ну очевидно по строке. А потом на другой машине ловите «No space left on device». Или «На диске не хватает места» или еще что-нибудь (потому что сообщения OS-зависимые). Или пытаетесь создать файл с определенным именем, а такой файл уже есть, просто у вас даже на чтение недостаточно прав чтобы его увидеть. Или просто ситуация когда не хватает прав — ничего вы этого понять в программе не можете, потому что все что вы имеете это IOException. Вот и получается что правильная последовательность действий одна — показать сообщение ошибки пользователю и дать на выбор «Повторить» или «Отменить».

wormball Jul 23 2018 at 23:28

Я всё же полагаю, что «никого не волнует причина» и «не всегда возможно точно определить причину» — это две большие разницы. Когда лично я копирую файлы и не могу их скопировать (да и вообще, совершаю какое-либо действие и не добиваюсь результата) — меня причина интересует всегда. Думаю, как и большинство здесь собравшихся.

dreamer-dead Jul 23 2018 at 12:08

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

UFO landed and left these words here

AndreyGaskov Jul 23 2018 at 13:22

Здесь проблема не в тестировании и тестируемости. Хотя и в этом тоже, так как для того, чтобы протестировать такую смесь UI, бизнес-правил и состояния окружения, надо написать кучу моков, выделить кучу интерфейсов и т.д. И получится вместо одного непонятного метода куча непонятных методов и классов.
Проблема в читаемости. Код пишется один раз, правится несколько раз, а читается иногда десятки и сотни раз. Прочитать написанное очень сложно: приходится постоянно прыгать в начало или конец метода, приходится постоянно проверять, что означают непонятные переменные, а также прослеживать все цепочки до конца кода. Возьмём, например, строку: «Actions userChoise = (A & D)? askUserForAction(event): null;». По каким причинам userAction может стать null? Потому что не (A & D). А что такое A? А что такое D? Возвращаемся в самое начало, чтобы выяснить. А может ли метод askUserForAction вернуть null, при том что (A &D)? Лезем смотреть реализацию askUserForAction. А если userAction реально станет null? Случится ли что-нибудь плохое? Надо проследить всё до самого конца, разобраться в каждой проверке, чтобы понять, что ничего страшного не произойдёт, хотя можно было бы сразу выйти.

UFO landed and left these words here

AndreyGaskov Jul 23 2018 at 13:57

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

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

UFO landed and left these words here

TerraV Jul 23 2018 at 22:24

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

myfunc Jul 23 2018 at 15:16

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

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

— Много переменных. Состояния мыши в можно объединить в структуру и передавать её между методами. Можно будет написать проверку не так:

boolean copyCase = (M & !K) | (A & !B & !C & !D & !K);

а, хотя бы так:

boolean copyCase = isCopyCase(mouseState);

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

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

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

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

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

HSerg Jul 24 2018 at 20:01

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