Время от времени я видел практику, которая «чувствует» неправильную, но я не могу четко сформулировать, что в ней плохого. Или, может быть, это просто мое предубеждение. Поехали:
Разработчик определяет метод с логическим значением в качестве одного из его параметров, и этот метод вызывает другой, и так далее, и в конечном итоге этот логический тип используется исключительно для определения того, следует ли предпринять определенное действие. Это может быть использовано, например, чтобы разрешить действие только в том случае, если у пользователя есть определенные права, или, возможно, если мы находимся (или нет) в тестовом режиме, в пакетном режиме или в режиме реального времени, или, возможно, только когда система находится в определенное состояние.
Ну, всегда есть другой способ сделать это, будь то путем запроса, когда пришло время выполнить действие (вместо передачи параметра), или с помощью нескольких версий метода, или нескольких реализаций класса и т. Д. Мой вопрос Не так много, как улучшить это, а скорее, действительно ли это неправильно (как я подозреваю), и если это так, то что неправильно в этом.
Ответы:
Да, это, вероятно, запах кода, который может привести к тому, что непонятный код будет трудно понять и у него будет меньше шансов на повторное использование.
Как отмечали другие авторы, контекст - это все (не входите в излишние правила, если это одноразовое решение, или если практика была признана преднамеренно возникшей технической задолженностью, которая будет повторно учтена позднее), но в целом, если есть параметр, переданный в функцию, которая выбирает конкретное поведение, которое должно быть выполнено, тогда требуется дальнейшее пошаговое уточнение; Разбиение этой функции на более мелкие функции приведет к более сплоченным функциям.
Так что же такое очень сплоченная функция?
Это функция, которая делает одно и только одно.
Проблема с параметром, переданным, как вы описываете, заключается в том, что функция выполняет более двух действий; он может проверять или не проверять права доступа пользователей в зависимости от состояния логического параметра, а затем в зависимости от этого дерева решений он будет выполнять часть функций.
Было бы лучше отделить проблемы контроля доступа от задач задачи, действия или команды.
Как вы уже отметили, переплетение этих проблем кажется неуместным.
Таким образом, понятие связности помогает нам определить, что рассматриваемая функция не очень связна и что мы могли бы реорганизовать код для получения набора более связных функций.
Таким образом, вопрос может быть переформулирован; Учитывая, что мы все согласны с тем, что лучше всего избегать передачи параметров поведенческого отбора, как мы можем улучшить ситуацию?
Я бы полностью избавился от параметра. Возможность отключения контроля доступа даже для тестирования является потенциальной угрозой безопасности. В целях тестирования либо заглушите, либо смоделируйте проверку доступа, чтобы протестировать сценарии как разрешенного, так и запрещенного доступа.
Ссылка: сплоченность (информатика)
источник
Я прекратил использовать эту модель давным-давно по очень простой причине; стоимость технического обслуживания. Несколько раз я обнаруживал, что у меня есть какая-то функция say,
frobnicate(something, forwards_flag)
которая была вызвана много раз в моем коде, и мне нужно было найти все места в коде, где значениеfalse
было передано как значениеforwards_flag
. Вы не можете легко найти их, поэтому это становится головной болью при обслуживании. И если вам нужно сделать исправление на каждом из этих сайтов, у вас может возникнуть неприятная проблема, если вы пропустите один.Но эта конкретная проблема легко решается без фундаментального изменения подхода:
С этим кодом нужно будет только искать экземпляры
FrobnicateBackwards
. Хотя возможно, что есть некоторый код, который присваивает это переменной, поэтому вы должны следовать нескольким потокам контроля, но на практике это довольно редко, и эта альтернатива работает нормально.Однако есть и другая проблема с передачей флага таким образом, по крайней мере, в принципе. Это связано с тем, что некоторые (только некоторые) системы, имеющие такой дизайн, могут раскрывать слишком много знаний о деталях реализации глубоко вложенных частей кода (который использует флаг) внешним уровням (которые должны знать, какое значение передать в этом флаге). Чтобы использовать терминологию Ларри Константина, эта конструкция может иметь слишком сильную связь между установщиком и пользователем логического флага. Фрэнки, хотя трудно сказать с какой-либо степенью уверенности в этом вопросе, не зная больше о кодовой базе.
Чтобы обратиться к конкретным примерам, которые вы приводите, у меня была бы некоторая обеспокоенность в каждом из них, но в основном по причинам риска / правильности. То есть, если вашей системе необходимо передать флаги, указывающие, в каком состоянии находится система, вы можете обнаружить, что у вас есть код, который должен был учесть это, но не проверять параметр (поскольку он не был передан эта функция). Таким образом, у вас есть ошибка, потому что кто-то пропустил передачу параметра.
Стоит также признать, что индикатор состояния системы, который необходимо передать почти каждой функции, на самом деле является глобальной переменной. Многие из недостатков глобальной переменной будут применяться. Я думаю, что во многих случаях лучше инкапсулировать знание состояния системы (или учетные данные пользователя, или идентификационные данные системы) в объекте, который отвечает за правильное действие на основе этих данных. Затем вы передаете ссылку на этот объект в отличие от необработанных данных. Ключевой концепцией здесь является инкапсуляция .
источник
bool
параметры были добавлены позже, и вызовы начинают выглядеть такDoSomething( myObject, false, false, true, false )
. Невозможно понять, что означают дополнительные логические аргументы, в то время как со значениями enum со значимыми именами это легко.Это не обязательно неправильно, но может представлять запах кода .
Основной сценарий, которого следует избегать в отношении логических параметров:
Затем, когда вы звоните, вы обычно звоните
foo(false)
и вfoo(true)
зависимости от того, какое поведение вы хотите.Это действительно проблема, потому что это случай плохой сплоченности. Вы создаете зависимость между методами, которая на самом деле не нужна.
То, что вы должны сделать в этом случае, это оставить
doThis
иdoThat
как отдельные и публичные методы затем сделать:или же
Таким образом, вы оставляете правильное решение вызывающей стороне (точно так же, как если бы вы передавали логический параметр) без создания связи.
Конечно, не все булевы параметры используются таким плохим образом, но это определенно запах кода, и вы правы, если вы заметите это в исходном коде.
Это только один пример того, как решить эту проблему на основе примеров, которые я написал. Есть и другие случаи, когда потребуется другой подход.
Есть хорошая статья Мартина Фаулера, в которой более подробно объясняется та же идея.
PS: если метод
foo
вместо вызова двух простых методов имеет более сложную реализацию, то все, что вам нужно сделать, это применить небольшой метод извлечения рефакторинга, чтобы результирующий код выглядел аналогично реализации,foo
которую я написал.источник
if (flag) doThat()
внутриfoo()
законно. Принятие решения о вызовеdoThat()
для каждого вызывающего абонента заставляет повторение, которое придется удалить, если вы позже узнаете о некоторых методах,flag
поведение также необходимо вызватьdoTheOther()
. Я бы предпочел поместить зависимости между методами в одном и том же классе, чем потом обыскивать всех вызывающих.doOne
иdoBoth
(для ложного и истинного случая, соответственно), либо использование отдельного типа перечисления, как предложено ДжеймсомdoOne()
илиdoBoth()
. Подпрограммы / функции / методы имеют аргументы, поэтому их поведение можно варьировать. Использование enum для истинно логического условия звучит очень похоже на повторение, если имя аргумента уже объясняет, что оно делает.Прежде всего: программирование - это не наука, а искусство. Так что очень редко есть «неправильный» и «правильный» способ программирования. Большинство стандартов кодирования - это просто «предпочтения», которые некоторые программисты считают полезными; но в конечном итоге они довольно произвольны. Поэтому я бы никогда не назвал выбор параметра «неправильным» сам по себе - и, конечно, не таким универсальным и полезным, как булев параметр. Использование
boolean
(илиint
, в этом отношении) для инкапсуляции состояния во многих случаях вполне оправдано.Решения по кодированию в целом должны основываться, прежде всего, на производительности и удобстве обслуживания. Если производительность не поставлена на карту (и я не могу себе представить, как это могло бы быть в ваших примерах), то ваш следующий вопрос должен быть следующим: насколько легко это будет для меня (или будущего редактора) поддерживать? Это интуитивно понятно и понятно? Он изолирован? Ваш пример вызовов связанных функций на самом деле кажется потенциально хрупким в этом отношении: если вы решите изменить его
bIsUp
наbIsDown
, сколько других мест в коде тоже нужно будет изменить? Кроме того, ваш список параметров раздувается? Если ваша функция имеет 17 параметров, то удобочитаемость является проблемой, и вам нужно пересмотреть вопрос о том, насколько вы цените преимущества объектно-ориентированной архитектуры.источник
Я думаю, что в статье, написанной Robert C Martins Clean, говорится, что вы должны исключать логические аргументы, где это возможно, поскольку они показывают, что метод делает больше, чем одно. Метод должен делать одно и только одно, я думаю, это один из его девизов.
источник
Я думаю, что самое важное здесь - это быть практичным.
Когда логическое значение определяет все поведение, просто создайте второй метод.
Когда логическое значение определяет только небольшое поведение в середине, вы можете захотеть сохранить его в одном, чтобы сократить дублирование кода. Там, где это возможно, вы можете даже разделить метод на три: два вызывающих метода для любого логического параметра и один, выполняющий основную часть работы.
Например:
Конечно, на практике вы всегда будете иметь точку между этими крайностями. Обычно я просто придерживаюсь того, что кажется правильным, но я предпочитаю ошибаться в сторону меньшего количества дублирования кода.
источник
FooInternal
в будущем, то что?Foo1
будет:{ doWork(); HandleTrueCase(); doMoreWork() }
. В идеале каждая из функцийdoWork
иdoMoreWork
должна быть разбита на (одну или несколько) значимых частей дискретных действий (т. Е. Как отдельные функции), а не просто две функции для разделения.Мне нравится подход настройки поведения с помощью методов построения, которые возвращают неизменяемые экземпляры. Вот как это использует Guava
Splitter
:Преимущества этого:
Splitter
. Вы бы никогда не добавилиsomeVaguelyStringRelatedOperation(List<Entity> myEntities)
класс с именемSplitter
, но вы бы подумали о том, чтобы поместить его как статический метод вStringUtils
класс.true
илиfalse
метод, чтобы получить правильное поведение.источник
Определенно запах кода . Если он не нарушает принцип единой ответственности, то, вероятно, нарушает принцип « говори, не спрашивай» . Рассмотреть возможность:
Если окажется, что он не нарушает один из этих двух принципов, вам все равно следует использовать enum. Булевы флаги являются логическим эквивалентом магических чисел .
foo(false)
имеет столько же смысла, сколькоbar(42)
. Перечисления могут быть полезны для паттернов стратегий и могут позволить вам добавить еще одну стратегию. (Только не забудьте назвать их соответствующим образом .)Ваш конкретный пример особенно беспокоит меня. Почему этот флаг проходит через так много методов? Похоже, вам нужно разделить ваш параметр на подклассы .
источник
TL; DR: не используйте логические аргументы.
Смотрите ниже, почему они плохие и как их заменить (жирным шрифтом).
Логические аргументы очень трудно читать, и, следовательно, их трудно поддерживать. Основная проблема заключается в том, что цель обычно ясна, когда вы читаете сигнатуру метода, в которой указан аргумент. Однако наименование параметра обычно не требуется в большинстве языков. Таким образом, у вас будут анти-паттерны, например,
RSACryptoServiceProvider#encrypt(Byte[], Boolean)
где логический параметр определяет, какой тип шифрования будет использоваться в функции.Таким образом, вы получите звонок как:
где читатель должен искать сигнатуру метода просто, чтобы определить, что на
true
самом деле может означать ад . Передача целого числа, конечно, так же плоха:скажет вам столько же, а точнее, так же мало. Даже если вы определите константы, которые будут использоваться для целого числа, пользователи функции могут просто игнорировать их и продолжать использовать литеральные значения.
Лучший способ решить эту проблему - использовать перечисление . Если вам нужно передать перечисление
RSAPadding
с двумя значениями:OAEP
илиPKCS1_V1_5
тогда вы сразу сможете прочитать код:Логическое значение может иметь только два значения. Это означает, что если у вас есть третий вариант, вам придется реорганизовать свою подпись. Обычно это не может быть легко выполнено, если проблема заключается в обратной совместимости, поэтому вам придется расширять любой открытый класс другим открытым методом. Это то, что Microsoft наконец сделала, когда они представили,
RSACryptoServiceProvider#encrypt(Byte[], RSAEncryptionPadding)
где они использовали перечисление (или, по крайней мере, класс, имитирующий перечисление) вместо логического значения.Может быть даже проще использовать полный объект или интерфейс в качестве параметра, если сам параметр должен быть параметризован. В приведенном выше примере само дополнение OAEP может быть параметризовано с использованием значения хеш-функции для внутреннего использования. Обратите внимание, что теперь существует 6 алгоритмов хеширования SHA-2 и 4 алгоритма хеширования SHA-3, поэтому число значений перечисления может взорваться, если вы используете только одно перечисление, а не параметры (возможно, это следующее, что Microsoft собирается выяснить ).
Логические параметры также могут указывать на то, что метод или класс не спроектированы должным образом. Как и в примере выше: любая криптографическая библиотека, кроме .NET, вообще не использует флаг дополнения в сигнатуре метода.
Почти все гуру программного обеспечения, которые мне нравятся, предостерегают от логических аргументов. Например, Джошуа Блох предостерегает их в высоко оцененной книге «Эффективная Ява». В общем, они просто не должны использоваться. Можно утверждать, что их можно использовать, если есть один параметр, который легко понять. Но даже тогда:
Bit.set(boolean)
вероятно, лучше реализовать двумя способами :Bit.set()
иBit.unset()
.Если вы не можете напрямую изменить код, вы можете определить константы, чтобы хотя бы сделать их более читабельными:
гораздо более читабельно, чем:
даже если бы вы предпочли:
вместо.
источник
Я удивлен, что никто не упомянул named-параметры .
Проблема, которую я вижу с логическими флагами, состоит в том, что они ухудшают читабельность. Например, что делает
true
вделать? Понятия не имею. Но что насчет:
Теперь ясно, для чего предназначен параметр, который мы передаем: мы говорим функции сохранить ее изменения. В этом случае, если класс не является общедоступным, я думаю, что логический параметр в порядке.
Конечно, вы не можете заставить пользователей вашего класса использовать named-параметры. По этой причине
enum
обычно предпочтительны один или два отдельных метода, в зависимости от того, насколько часто используется ваш случай по умолчанию. Это именно то, что делает .Net:источник
myFunction(true)
может быть написан такой код, как код запах.SaveBig
имя. Любой код может быть испорчен, этот вид не относится к именованным параметрам.Если он выглядит как запах кода, ощущается как запах кода и - ну, хорошо пахнет запахом кода, это, вероятно, запах кода.
Что вы хотите сделать, это:
1) Избегайте методов с побочными эффектами.
2) Обрабатывать необходимые состояния с помощью центрального, формального автомата (как это ).
источник
Я согласен со всеми проблемами использования логических параметров, чтобы не определять производительность, чтобы; улучшение, удобочитаемость, надежность, снижение сложности, снижение рисков из-за плохой инкапсуляции и сплоченности, а также снижение совокупной стоимости владения с помощью обслуживания.
Я начал проектировать аппаратное обеспечение в середине 70-х годов, которое мы теперь называем SCADA (диспетчерское управление и сбор данных), и это было точно настроенное аппаратное обеспечение с машинным кодом в EPROM, работающее с макро-пультами дистанционного управления и собирающее высокоскоростные данные.
Логика называется машинами Мили и Мура, которые мы сейчас называем конечными автоматами . Это также должно быть сделано по тем же правилам, что и выше, если только это не машина реального времени с конечным временем выполнения, а затем для достижения цели должны быть созданы ярлыки.
Данные являются синхронными, но команды являются асинхронными, и логика команд следует булевой логике без памяти, но с последовательными командами, основанными на памяти предыдущего, настоящего и желаемого следующего состояния. Чтобы это работало на самом эффективном машинном языке (всего 64 КБ), была предпринята большая осторожность, чтобы определить каждый процесс в эвристическом стиле IBM HIPO. Иногда это означало передачу булевых переменных и выполнение индексированных ветвей.
Но теперь, с большим объемом памяти и простотой OOK, Encapsulation является важным компонентом сегодня, но штрафом, когда код подсчитывается в байтах для реального времени и машинного кода SCADA.
источник
Это не обязательно неправильно, но в вашем конкретном примере действия, зависящего от некоторого атрибута «пользователя», я бы пропустил ссылку на пользователя, а не на флаг.
Это проясняет и помогает несколькими способами.
Любой, кто читает оператор вызова, поймет, что результат будет меняться в зависимости от пользователя.
В функции, которая в конечном итоге вызывается, вы можете легко реализовать более сложные бизнес-правила, поскольку вы можете получить доступ к любому из атрибутов пользователя.
Если одна функция / метод в «цепочке» делает что-то другое в зависимости от пользовательского атрибута, очень вероятно, что аналогичная зависимость от пользовательских атрибутов будет введена для некоторых других методов в «цепочке».
источник
Большую часть времени я бы посчитал это плохим кодированием. Однако я могу вспомнить два случая, когда это может быть хорошей практикой. Поскольку уже есть много ответов о том, почему это плохо, я предлагаю два раза, когда это может быть хорошо:
Первый, если каждый вызов в последовательности имеет смысл сам по себе. Было бы разумно, если бы вызывающий код мог быть изменен с true на false или false на true, или если вызываемый метод мог бы быть изменен для непосредственного использования логического параметра вместо его передачи. Вероятность десяти таких вызовов подряд невелика, но это может произойти, и если это произойдет, это будет хорошей практикой программирования.
Второй случай немного натянут, учитывая, что мы имеем дело с логическим значением. Но если в программе имеется несколько потоков или событий, передача параметров - это самый простой способ отслеживать данные, специфичные для потока / события. Например, программа может получать данные из двух или более сокетов. Код, работающий для одного сокета, может выдавать предупреждающие сообщения, а один для другого - нет. Тогда (в некотором роде) имеет смысл для логического набора на очень высоком уровне проходить через множество вызовов методов в те места, где могут генерироваться предупреждающие сообщения. Данные не могут быть сохранены (кроме как с большим трудом) в каком-либо глобальном виде, потому что каждый поток или чередующиеся события будут нуждаться в собственном значении.
Безусловно, в последнем случае я бы, вероятно, создал класс / структуру, единственным содержимым которой был логический тип, и вместо этого передал бы это . Я был бы почти уверен, что скоро понадобятся другие поля, например, куда отправлять предупреждающие сообщения.
источник
Контекст важен. Такие методы довольно распространены в iOS. В качестве одного из часто используемых примеров UINavigationController предоставляет метод
-pushViewController:animated:
, аanimated
параметр представляет собой BOOL. Метод выполняет в основном ту же самую функцию в любом случае, но он оживляет переход от одного контроллера представления к другому, если вы передаете YES, и не делает, если вы передаете NO. Это кажется вполне разумным; было бы глупо предоставлять два метода вместо этого, чтобы вы могли определить, использовать анимацию или нет.Это может быть проще для обоснования такого рода вещей в Objective-C, где синтаксис именования методов обеспечивает больше контекста для каждого параметра, чем в таких языках, как C и Java. Тем не менее, я думаю, что метод, который принимает один параметр, может легко принять логическое значение и все же иметь смысл:
источник
false
означает вfile.saveWithEncryption
примере. Значит ли это, что он будет сохранять без шифрования? Если так, то почему в мире этот метод имеет «с шифрованием» право на название? Я мог бы понять, что есть такой методsave(boolean withEncryption)
, но когда я вижуfile.save(false)
, на первый взгляд совсем не очевидно, что параметр указывает, что он будет с или без шифрования. Я думаю, на самом деле, это делает первое замечание Джеймса Янгмана об использовании enum.false
, что не перезаписывайте любой существующий файл с таким же именем. Придуманный пример я знаю, но чтобы быть уверенным, вам нужно проверить документацию (или код) для функции.saveWithEncryption
который иногда не сохраняется с шифрованием, является ошибкой. Должно бытьfile.encrypt().save()
, или как Javasnew EncryptingOutputStream(new FileOutputStream(...)).write(file)
.saveWithEncryption(boolean)
который иногда сохраняет без шифрования, точно так же, как он не может создать метод ,saveWithoutEncryption(boolean)
который иногда сохраняет с шифрованием.