Правильный комментарий для аргументов логической функции, которые являются «ложными»?

19

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

void someFunction(bool forget);

void ourFunction() {
  someFunction(false /* forget */);
}    

Я всегда сомневаюсь, что falseздесь значит. Означает ли это «забыть», или «забыть» относится к соответствующему параметру (как в случае выше), а «ложь» означает его отрицание?

Какой стиль используется чаще всего, и как лучше (или некоторые из лучших способов) избежать двусмысленности?

Йоханнес Шауб - Литб
источник
38
используйте перечисления (даже если есть только 2 варианта) вместо bools
Esailija
21
Некоторые языки поддерживают именованные аргументы. На таких языках вы могли бы использоватьsomeFunction(forget: true);
Брайан
3
Я чувствую себя обязанным предоставить аргументы Мартина Фаулера о аргументах флага (также говорит о булевых установщиках). В общем, старайтесь избегать их.
FGreg
3
Чтобы отметить очевидное, комментарии могут лгать. Таким образом, всегда лучше , чтобы сделать ваш код самодокументируемым, потому что некоторые из них будут меняться , trueчтобы falseи не обновлять свой комментарий. Если вы не можете изменить API, то лучший способ это прокомментироватьsomeFunction( false /* true=forget, false=remember */)
Марк Лаката
1
@Bakuriu - на самом деле, у меня, вероятно, все еще было бы, чтобы у общедоступного API было два отдельных метода ( sortAscendingи / sortDescendingили аналогичных). Теперь внутри они оба могут вызывать один и тот же закрытый метод, который может иметь такой тип параметра. На самом деле, если бы язык поддерживал это, вероятно, что я передал бы лямбда-функцию, которая содержала бы направление сортировки ...
Clockwork-Muse

Ответы:

33

В приведенном вами примере кода это выглядит как forgetаргумент флага. (Я не могу быть уверен, потому что функция является чисто гипотетической.)

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

Чтобы избежать аргумента флага, разбейте функцию на две функции, которые объясняют разницу в имени функции.

Флаг аргумента

serveIceCream(bool lowFat)

Нет аргумента флага

serveTraditionalIceCream()
serveLowFatIceCream()

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


Это всего лишь догадка, но параметр с именем forgetзвучит как зависть к функциям. Почему звонящий сказал другому объекту забыть что-нибудь? Там может быть большая проблема дизайна.

Аарон Куртжалс
источник
4
+17, шлем включен. Что делать тела serveTraditionalIceCreamи serveLowFatIceCreamвыглядеть? У меня есть enum с 14 видами мороженого.
JohnMark13
13
Для открытых методов это соглашение хорошо, но, как намекает ДжонМарк, другая половина SRP - «хороший метод должен быть единственным, который делает то, что делает». Я вижу N + 1 вариантов этого метода; N public без параметров, каждый из которых вызывает один закрытый метод с параметром, который вы пытаетесь избежать раскрытия. В какой-то момент вы просто сдаетесь и выставляете проклятый параметр. Запахи кода не обязательно означают, что код должен быть реорганизован; это просто вещи, которые заслуживают второго взгляда в обзоре кода.
KeithS
@ Аарон Под "завистью к чертям", что ты имел ввиду?
Компьютерщик
27

По словам непрофессионала:

  • false это буквальное.
  • вы передаете буквальный false
  • ты говоришь someFunctionне забывать
  • Вы говорите, someFunctionчто параметр забудьfalse
  • ты говоришь someFunctionпомнить

На мой взгляд, было бы лучше, если бы функция была такой:

void someFunction(bool remember);

Вы можете назвать это

void ourFunction() {
  someFunction(true);
} 

или сохраните старое имя, но измените функцию-оболочку на

void ourFunctionWithRemember() {
  someFunction(false);
} 

РЕДАКТИРОВАТЬ:

Как сказал @Vorac, всегда старайтесь использовать позитивные слова. Двойное отрицание сбивает с толку.

Тулаинс Кордова
источник
15
+1 за идею всегда стремиться использовать позитивные слова. Двойное отрицание сбивает с толку.
Vorac
1
Согласен, отличная идея. Сказать, что система не делает что-то, сбивает с толку. Если вы принимаете логический параметр, выражайте его положительно.
Брэндон
В большинстве случаев, я думаю, вы также хотели бы быть более конкретным remember, если только имя функции не сделало значение remember очень очевидным. rememberToCleanUp* or *persistили что-то.
Брюс
14

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

Использование положительного имени переменной (предлагается в Code Complete ) может быть самым простым изменением, которое облегчит чтение этого фрагмента, например

void someFunction(boolean remember);

затем ourFunctionстановится:

void ourFunction() {
    someFunction(true /* remember */);
}

Однако использование enum делает вызов функции еще проще для понимания за счет некоторого вспомогательного кода:

public enum RememberFoo {
    REMEMBER,
    FORGET
}

...

void someFunction(RememberFoo remember);

...

void ourFunction() {
    someFunction(RememberFoo.REMEMBER);
}

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

void someFunction(boolean remember);

...

void ourFunction() {
    boolean remember = false;
    someFunction(remember);
}
Майк Партридж
источник
1
Установка rememberв true означает забыть (в вашем примере someFunction(true /* forget */);)?
Брайан,
2
На enumсегодняшний день это лучшее решение. То, что тип может быть представлен как «то boolесть», они изоморфны - это не значит, что он должен быть представлен как таковой. Тот же аргумент относится stringи к даже int.
Джон Пурди,
10

Переименуйте переменную, чтобы значение bool имело смысл.

Это в миллион раз лучше, чем добавление комментария для объяснения аргументов функции, потому что имя неоднозначное.

enderland
источник
3
Это не отвечает на вопрос. Все очевидно, когда метод определен и вызван в одном и том же файле на 4 строки. Но что, если все, что вы видите в данный момент, это вызывающий абонент? Что делать, если он имеет несколько логических значений? Иногда простой встроенный комментарий имеет большое значение.
Брэндон
@Brandon Это не аргумент против вызова логического параметра doNotPersist (или, лучше, Persist). Называть это «забыть», не говоря, что забыть, откровенно бесполезно. О, и метод, который использует несколько логических значений в качестве опции, воняет в рай.
Брюс
5

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

void ourFunction() {
    bool takeAction = false;  /* false means to forget */
    someFunction( takeAction );
}    

Если вы не можете переименовать переменную, то комментарий должен быть немного более выразительным:

void ourFunction() {
    /* false means that the method should forget what was requested */
    someFunction( false );
}    

источник
1
Определенно хороший совет, но я не думаю, что он решает проблему, /* forget */к которой относится этот комментарий, а именно, что без объявления функции прямо перед вами может быть трудно вспомнить, что устанавливается false. (Вот почему я думаю, что совет @ Esailija лучше добавить перечисление, и поэтому я люблю языки, которые допускают именованные параметры.)
Gort the Robot
@StevenBurnap - спасибо! Вы правы в том, что мой старый ответ не был достаточно ясен при рассмотрении вопроса ОП. Я отредактировал это, чтобы сделать это более ясным.
3

Есть хорошая статья, в которой упоминается именно эта ситуация, так как она относится к API Qt-Style. Там она называется Ловушка булевых параметров, и ее стоит прочитать.

Суть этого заключается в:

  1. Перегрузка функции, так что bool не нужен, лучше
  2. Лучше всего использовать enum (как предлагает Esailija)
Стивен Эверс
источник
2

Это странный комментарий.

На someFunction(false /* forget */);самом деле, с точки зрения компилятора someFunction(false);(комментарий удаляется). Итак, все, что делает эта строка, это вызов someFunctionс первым (и единственным) аргументом, установленным в false.

/* forget */это просто имя параметра. Это, вероятно, не более чем быстрое (и грязное) напоминание, которое на самом деле не должно быть там. Просто используйте менее двусмысленное имя параметра, и вам вообще не понадобится комментарий.

user2590712
источник
1

Один из советов чистого кода - минимизировать количество ненужных комментариев 1 (потому что они имеют тенденцию гнить) и правильно называть функции и методы.

После этого я бы просто удалил комментарий. В конце концов, современные IDE (например, eclipse) выдают окно с кодом, когда вы наводите курсор мыши на функцию. Увидев код, следует убрать неоднозначность.


1 Комментировать некоторый сложный код - это нормально.

BЈовић
источник
Кстати, кто сказал что-то вроде этого: «худшая проблема программиста - как назвать переменную и сместить ее на единицу»?
BЈовић
4
Вы, вероятно, ищете источник martinfowler.com/bliki/TwoHardThings.html . Я слышал твик: «В компьютерных науках есть только две сложные вещи: аннулирование кэша, присвоение имен и отключение из-за одной ошибки».
1

Чтобы отметить очевидное, комментарии могут лгать. Таким образом, всегда лучше , чтобы сделать ваш код самодокументируемыми , не прибегая к комментариям объяснить, потому что какой - то человек (возможно , вы) изменится trueна , falseа не обновлять свой комментарий.

Если вы не можете изменить API, то я прибегаю к 2 вариантам

  • Измените комментарий, чтобы он всегда был верным, независимо от кода. Если вы называете это только один раз, это хорошее решение, поскольку документ остается локальным.
     someFunction (false / * true = забыть, false = запомнить * /); `
  • Используйте #defines, особенно если вы вызываете его более одного раза.
     #define FORGET true
     #define REMEMBER false
     SomeFunction (REMEMBER);
Марк Лаката
источник
1

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

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

jmoreno
источник
-1

В C # я использовал именованные параметры, чтобы сделать это более понятным

someFunction(forget: false);

Или enum:

enum Memory { Remember, Forget };

someFunction(Memory.Forget);

Или перегрузки:

someFunctionForget();

Или полиморфизм`

var foo = new Elephantine();

foo.someFunction();
Джей Базузи
источник
-2

Именование должно всегда разрешать неопределенность для логических значений. Я всегда называю логическое что-то вроде «isThis» или «shouldDoThat», например:

void printTree(Tree tree, bool shouldPrintPretty){ ... }

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

dchhetri
источник
как это отвечает на заданный вопрос?
комнат
@gnat Я отвечал на его вопрос о разрешении двусмысленности с булевыми параметрами. Может, я неправильно прочитал его вопрос.
декабря