Сейчас предлагаю разобрать повторяющийся случай с обилием ветвлений.
Задание
Пользователь намеревается перетащить файл мышкой из одного окна папки в другое. Нужно написать метод-диспетчер, который проверит суть события и возможность его обработки, если нужно, уточнит детали, затем вызовет нужный метод или выдаст сообщение об ограничениях.
Если пользователь перетащил и отпустил из папки в папку и другая папка находится на другом разделе, то проверить возможность копирования. Если скопировать можно, то скопировать. Иначе выдать сообщение, что нельзя скопировать. Невозможно скопировать бывает по причинам: нет прав на запись; не хватает свободного места; файловая система не поддерживает символы в имени; имя файла в папке назначения будет иметь слишком длинный путь; в папке уже есть файл с таким именем (вызвать диалог на перезапись файла, если пользователь согласен, то перезаписать).
Если папка назначения находится на том же разделе, что и файл, то переместить файл. Невозможно переместить: нет прав на запись; полный путь назначения окажется слишком длинным, в папке уже есть файл с тем же именем (вызвать диалог); файл системный и его нельзя удалить; уже есть файл с таким имененем (вызвать диалог на перезапись файла, если пользователь согласен, то перезаписать).
Если пользователь перенес файл в другой окно, но оно имеет тот же путь, то создать копию файла (Добавить к имени « копия #», где # – наименьшее положительное число, делающее файл уникальным). Невозможно создать копию: нет прав на запись; полный путь слишком длинный; не хватает свободного места.
Если пользователь переносил правой кнопкой, то вызвать диалог для выбора действия (скопировать/переместить/создать ярлык/создать копию).
Если пользователь отпустил файл в том же окне (файл сорвался) левой кнопкой, то ничего не делать. А если правой, то предложить создать копию или ярлык. Если файл сорвался не в окне папки, то ничего не делать.
Со временем могут появиться новые условия, новые действия, могут измениться уже описанные условия для выбора действий.
Решение для обсуждения
Предлагаю своё спорное решение на 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)
The_Pro
23.07.2018 07:32+1По именованию переменных жесть конечно. Такое чувство как будто в один момент стало лень придумывать названия.
boolean isPossibleToCopyRewrite = isPossibleToCopy & L;
hVostt
23.07.2018 08:17+1Нужно определить или использовать существующую структуру для валидации, метод должен проверять условия и возвращать экземпляр структуры с результатами валидации. Никакой конкатенации строк, коллекции. Метод валидации не должен показывать никаких окон.
akryukov
23.07.2018 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);AMURWOLF Автор
23.07.2018 09:23Вы пришли к тому, от чего я убежал. Раньше я делал либо 10-уровневые деревья, либо сочетания 5-уровневых деревьев с вынесенными отдельными поддеревьями с досрочным return. Во время изменения требований я путался и делал ошибки, так как приходилось держать в уме ситуации досрочных выходов и при необходимости возвращать вынесенные отдельные поддеревья в единое общее дерево. Каждый раз приходиться пересматривать все ветки и все вынесенные поддеревья, это было утомительно. Ведь когда-нибудь появится действие на предикат! А с сочетанием с другим предикатом и т.д.
AMURWOLF Автор
23.07.2018 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
23.07.2018 09:45+1Лучше бы не убегали. В предикатах вроде
(M & !K) | (A & !B & !C & !D & !K);запутаться так же просто, как и в "лесу" из if'ов.
Если предлагаемый вариант не подходит, то попробуйте каждую ситуацию вынести в свой класс с методами "canBeExecuted(event)" и "execute(event)". Экземпляры ситуаций положите в коллекцию и при каждом событии вызывайте execute первой стратегии, у которой canBeExecuted вернет true. Внутри execute можно будет написать соответствующие обработчики ошибок.
maxzh83
23.07.2018 09:39Просится выделение отдельного метода валидации, который кроме проверки ничего не делает. Еще хочется более осмысленного названия переменных, хотя бы мнемоники, т.е. вместо А — TPIDW, ну или чего-то получше. Также повторяющиеся куски ((A & !B & C & !D & K)) вынести и дать осмысленные названия.
omickron
23.07.2018 10:17Я бы завернул такой PR.
1. В логику булевых операций вмешан запрос подтверждения от пользователя askUserWantToRewrite(). Это увеличивает связность и без того сложного метода
2. Результат метода — действие с файлом или показ сообщения.
По-хорошему нужно выделить отдельный метод на проверку всех параметров. Метод будет возвращать признак операции, можно ли её совершить, признак необходимости подтверждения пользователя и причину, почему выполнить нельзя.
Результат метода передаётся в соответствующий обработчик действия, которых должно быть столько, сколько есть возможных действий.
Тогда будет легко покрыть тестами и метод проверки условий, и каждый конкретный обработчик.
P. S. Не вижу проблемы в названиях переменных. Это тот исключительный случай, когда названиями переменных лучше пожертвовать для понимания сложных булевых операций.
NickViz
23.07.2018 10:22+1«другая папка находится на другом разделе, то проверить возможность копирования. Если скопировать можно, то скопировать. Иначе выдать сообщение, что нельзя скопировать.»
Вот не надо так делать:
А) лишняя проверка, всё равно при копировании будет сделано тоже самое системой.
Б) бессмысленная проверка, перед копированием, но после проверки возможность может исчезнуть (кончилось место, например)
Поэтому надо копировать и разбирать код возврата. Реймон Чен (ЕМНИП) об этом говорил.
AndreyGaskov
23.07.2018 10:51+1Перед тем, как менять названия переменных и наводить красоту, было бы хорошо поработать над тем, чтобы отделить UI от логики. В функции сначала вычисляются предикаты, а потом 1 или 2 раза запрашиваются действия пользователя: askUserForAction(event) и askUserWantToRewrite(). Пользователь может не отвечать довольно долго. За то время, пока он решит ответить, ситуация в окружении может измениться значительно: может закончиться место, кто-нибудь другой может добавить файл с таким же названием в директорию назначения, директория назначения может быть вообще удалена. Да и как-то это неправильно смешивать бизнес-правила с сценарием UI.
lair
23.07.2018 11:37Будьте проще. Нет, намного проще.
- Сначала надо определить совершаемое действие, и только его: либо это перенос, либо это копирование, либо дублирование (дублирование — это, в данном случае, копирование файла в файл с именем "… копия x"). Если перенос был левой кнопкой, действие определяется автоматически, если правой — по выбору пользователя (а "автоматическое" используется как дефолт).
- Запустить соответствующую операцию, если произошла одна из перечисленных ошибок, разбираем ее.
- Ошибки делятся на те, где от пользователя нужен дальнейший ввод, и все остальные (retry-cancel мы за ввод не считаем, это общая стратегия); у дублирования ошибок-со-вводом не бывает, у остальных операций это ситуация "файл с таким именем уже существует".
- Если пользователь задал новое имя, запускаем ту же операцию, но с явным указанием целевого имени, и возвращаемся на шаг 2.
PS Если речь идет о Java или любом другом ОО-языке, каждая операция — это объект, а повторного использования кода можно добиться наследованием или композицией, в зависимости от ваших архитектурных предпочтений и языка.
YoungSkipper
23.07.2018 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
23.07.2018 12:07+1Довольно типичный пример многоуровневого косяка. Начинается всё с бизнес-логики (помесь астрологии и херомантии — когда луна в сатурне делаем то-то). Одно из главных правил разработки пользовательского интерфейса — консистентность. Перетаскивание файла должно приводить только к одному действию вне зависимости от гороскопа пользователя. В Windows это слегка нарушено (в пределах одного диска это перемещение, разные диски — копия). В зависимости от модификаторов (нажатая кнопка контрол) это может быть копирование или (нажатая кнопка альт) создание ярлыка или (перетаскивание правой кнопкой мыши) контекстное меню. Имея четкие правила изначально, вся логика укладывается в простейший try-catch.
Идем дальше, как уже справедливо заметили выше, ранние проверки в лучшем случае бесполезны а по факту даже вредны (так как ухудшают читаемость и плодят немеряную сложность на ровном месте). Меньшая часть логики должна сидеть в блоке обработки ошибок, большая часть должна быть просто удалена. Никого не волнует причина по которой не может быть выполнено копирование (дисковое пространство, права доступа или водолей в овне).wormball
23.07.2018 19:09> Никого не волнует причина по которой не может быть выполнено копирование
Смелое заявление.TerraV
23.07.2018 22:22Я скажу даже смелее. Вы никогда не можете быть уверены что верно идентифицировали причину, по которой копирование невозможно. К примеру вы поймали IOException с сообщением «There is not enough space on the disk». Нормально, на диске не хватает места. Как привязаться — ну очевидно по строке. А потом на другой машине ловите «No space left on device». Или «На диске не хватает места» или еще что-нибудь (потому что сообщения OS-зависимые). Или пытаетесь создать файл с определенным именем, а такой файл уже есть, просто у вас даже на чтение недостаточно прав чтобы его увидеть. Или просто ситуация когда не хватает прав — ничего вы этого понять в программе не можете, потому что все что вы имеете это IOException. Вот и получается что правильная последовательность действий одна — показать сообщение ошибки пользователю и дать на выбор «Повторить» или «Отменить».
wormball
23.07.2018 23:28Я всё же полагаю, что «никого не волнует причина» и «не всегда возможно точно определить причину» — это две большие разницы. Когда лично я копирую файлы и не могу их скопировать (да и вообще, совершаю какое-либо действие и не добиваюсь результата) — меня причина интересует всегда. Думаю, как и большинство здесь собравшихся.
dreamer-dead
23.07.2018 12:08+2Есть чуть более специализированные ресурсы для этого, например codereview.stackexchange.com
appsforlife
23.07.2018 12:31-1Я, когда упираюсь в такого рода задачи, предпочитаю не заморачиваться над идеальным дизайном вот этого конкретного метода, а просто обвешиваю его тестами, проверяющими все стандартные сценарии, и оставляю как есть.
Обычно в процессе обвешивания вымывается всякая муть типа показа сообщений или запросов подтверждений у пользователя, появляется возможность добавить моки для таких «внешних» по отношению к алгоритму операций. В итоге получаем слегка улучшенный и покрытый тестами код. А что там внутри — да пофиг, если работает и не течет/тормозит.
Если поступают новые требования, то под них пишутся новые тесты, а потом алгоритм минимально адаптируется, чтобы их проходить.
Для такого кода, как в статье, метод сработает на ура.AndreyGaskov
23.07.2018 13:22Здесь проблема не в тестировании и тестируемости. Хотя и в этом тоже, так как для того, чтобы протестировать такую смесь UI, бизнес-правил и состояния окружения, надо написать кучу моков, выделить кучу интерфейсов и т.д. И получится вместо одного непонятного метода куча непонятных методов и классов.
Проблема в читаемости. Код пишется один раз, правится несколько раз, а читается иногда десятки и сотни раз. Прочитать написанное очень сложно: приходится постоянно прыгать в начало или конец метода, приходится постоянно проверять, что означают непонятные переменные, а также прослеживать все цепочки до конца кода. Возьмём, например, строку: «Actions userChoise = (A & D)? askUserForAction(event): null;». По каким причинам userAction может стать null? Потому что не (A & D). А что такое A? А что такое D? Возвращаемся в самое начало, чтобы выяснить. А если userAction реально станет null? Надо проследить всё до самого конца, понять каждую инструкцию, чтобы понять, что ничего страшного не произойдёт, хотя можно было бы сразу выйти.appsforlife
23.07.2018 13:51Да это понятно, тут дело не в названиях переменных а в самом подходе к решению таких задач. Будут там вложенные if-ы или всякие логические комбинации флагов — не суть. Как будут названы переменные — не критично. Все равно программист, когда придется править этот код, будет сидеть и втыкать какое-то время что там к чему. Потом он в этом контексте сможет поправить код вне зависимости от того как конкретно он будет написан, а тесты не дадут накосячить. Дело в тестах, а не в названиях переменных. Тесты это идеальный способ описать черный ящик алгоритма, этим надо пользоваться в первую очередь.
И неправда Ваша, что код читается десятки раз. Конкретно этот код будет написан и забыт как страшный сон, пока что-то не потребуется. Очень редкий код читается много раз, если он закончен. Смысл его читать? Вот если его меняют постоянно, тогда да. Но тогда это или развивающийся код, или хреновый код, раз в нем регулярно копаются. В первом случае без вариантов (но это не случай из статьи), во втором надо начинать с тестов, потом хреновость постепенно выветрится. В статье код замороченный, но не хреновый. Могло быть ощутимо хуже.
В данном конкретном случае я бы заменил кучу вызовов в самом начале на подаваемую извне структуру данных, вместо выполнения операции копирования/переноса возвращал бы что надо делать, а вызовы гуя (там вроде их два) сделал бы через интерфейс «IГуй», к которому и потребовался бы несложный мок. Потом поверх бы сделал кучу тестов на все сценарии. Впрочем, к такому коду и так нужна куча тестов, без них никуда. А с кучей тестов уже не так и важно что там за код внутри.
А так конечно хорошо быть эльфом в стране эльфов и всегда писать идеальный код, но задач много, времени мало и иногда чуток говнокода, здоровый цинизм и юнит-тесты позволяют сворачивать горы гораздо меньшими усилиями.AndreyGaskov
23.07.2018 13:57А так конечно хорошо быть эльфом в стране эльфов и всегда писать идеальный код, но задач много, времени мало и иногда чуток говнокода, здоровый цинизм и юнит-тесты позволяют сворачивать горы гораздо меньшими усилиями.
В данном случае можно было бы потратить столько же времени и написать более читаемый код, который было бы гораздо проще и приятнее покрывать тестами. И автор совершенно правильно делает, что хочет научиться делать лучше, вынося свой код на ревью общественности. Не у каждого смелости хватит.appsforlife
23.07.2018 14:05Тестами покрываться этот код будет одинаково, вне зависимости от его содержания, ведь количество тестируемых сценариев не зависит от качества кода (в разумных пределах).
Я не призываю говнокодить, я призываю не тратить много времени на достижение идеала. Есть критерии расхода памяти, есть критерии скорости, есть набор тестов — все. Дальше чем быстрее ты реализуешь алгоритм, тем лучше. Если при этом получится красиво и удобно — вообще зашибись, не получится — ну и ладно.
Понятно, что речь идет о промышленной разработке. Если это дома и для души, то можно неделю посидеть, подумать и сделать шедевр. Но это дома и исключительно за свой счет. Как-то так.omickron
23.07.2018 14:53Тестами покрываться этот код будет одинаково, вне зависимости от его содержания
К сожалению, не совсем, ведь код содержит и логику проверки, и взаимодействие с пользователем, и взаимодействие с файловой системой.
Если разнести всё по отдельным классам, то и покрывать тестами будет намного проще.
TerraV
23.07.2018 22:24Прежде чем давать совет «покрывать всё тестами», было бы неплохо спросить себя — а стал бы я покрывать тестами такой код? И что-то мне кажется что в этом случае ответ будет «нет» — время на заведомую ерунду тратить жалко.
myfunc
23.07.2018 15:16Этот подход для новичка может показаться нормальным, но он имеет очень много «косяков», и не подходит для больших проектов. Давай по порядку:
— Имена переменных. Выше обсуждалось, код должен быть читабельным без комментариев.
— Большая функция. Выходит больше строк кода, что значит больше переменных, а значит длиннее их имена. Я бы выносил логические блоки в другие методы, даже если метод будет вызываться всего один раз.
— Много переменных. Состояния мыши в можно объединить в структуру и передавать её между методами. Можно будет написать проверку не так:
boolean copyCase = (M & !K) | (A & !B & !C & !D & !K);
а, хотя бы так:
boolean copyCase = isCopyCase(mouseState);
где в isCopyCase я бы скомбинровал проверку правильным образом.
— Код будет редактироваться другими членами команды, и чтобы оставить его «читабельным», они должны писать так-же неправильно. И скорее всего они не будут следовать твоим внутренним правилам, и через 3-4 сторонние правки вряд-ли сам поймешь, что происходит в коде.
— Для постройки строк оптимальнее использовать StringBuilder.
— Правильное разделение кода на пакеты, классы, методы дает возможность покрывать код тестами. Код например можно разделить на блоки: «вычитка состояния», «проверка», «выполнения действия». И нет ни одной причины писать всё это в одном вместе.
В общем, есть интересная книжка — «Боб Мартин — Чистый код», она перевернёт твой взгляд, как однажды перевернула и мой. Советую почитать.
HSerg
24.07.2018 20:01Подключите к сборке checkstyle & pmd и попробуйте некоторое время обходиться без suppress-ов или отключения правил.
Valle
А теперь, не подглядывая в код, что значит "(M & !K) | (A & !B & !C & !D)"?
AMURWOLF Автор
Как предлагаете записать иначе? Условие может быть очень длинным, например таким: !(A & H) & (K | !D | (F & D) | !(E & !G)) & Z & !Y
При этом каждая буква может расшифровываться как очень длинное предложение, если дать нормальное имя, то оно будет состоять из нескольких слов, пяти, шести. Так хотя бы видны дизъюнкции, конъюнкции, отрицания и скобки. Расшифровка букв всегда рядом.
peresada
Расшифровка — это костыль, хорошему программисту следует сделать все, чтобы его код можно было прочитать без дополнительных расшифровок. Пусть он будет длиннее на порядок, но читабельней, да и с длинными предложениями никто не мешает поступать переносом каждого условия с нормальным форматированием