Однострочные функции, которые вызываются только один раз

120

Рассмотрим функцию без параметров ( edit: необязательно), которая выполняет одну строку кода и вызывается в программе только один раз (хотя не исключено, что она понадобится снова в будущем).

Он может выполнить запрос, проверить некоторые значения, сделать что-то с использованием регулярных выражений ... что-то неясное или «хакерское».

Основанием для этого было бы избежать трудно читаемых оценок:

if (getCondition()) {
    // do stuff
}

где getCondition()однострочная функция

Мой вопрос просто: это хорошая практика? Мне кажется, что все в порядке, но я не знаю о долгосрочной перспективе ...

vemv
источник
9
Если ваш фрагмент кода не написан на языке ООП с неявным получателем (например, этим), это, безусловно, будет плохой практикой, так как getCondition (), скорее всего, зависит от глобального состояния ...
Ingo
2
Возможно, он подразумевал, что getCondition () может иметь аргументы?
user606723
24
@ Инго - некоторые вещи действительно имеют глобальное состояние. Текущее время, имена хостов, номера портов и т. Д. Являются действительными глобальными значениями. Ошибка проектирования делает Global из чего-то, что по своей природе не является глобальным.
Джеймс Андерсон
1
Почему бы тебе просто не вставить getCondition? Если он настолько мал и редко используется, как вы говорите, то присвоение ему имени ничего не значит.
davidk01
12
davidk01: читабельность кода.
WJL

Ответы:

240

Зависит от этой одной строки. Если строка читаема и кратка сама по себе, функция может не понадобиться. Упрощенный пример:

void printNewLine() {
  System.out.println();
}

OTOH, если функция дает хорошее имя для строки кода, содержащей, например, сложное, трудно читаемое выражение, это совершенно оправдано (для меня). Придуманный пример (разбит на несколько строк для удобства чтения):

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}
Петер Тёрёк
источник
99
+1. Волшебное слово здесь - «Самодокументированный код».
Konamiman
8
Хороший пример того, что дядя Боб назвал бы «инкапсулированием условных выражений».
Энтони Пеграм,
12
@ Aditya: Ничто не говорит, что taxPayerявляется глобальным в этом сценарии. Возможно, этот класс есть TaxReturnи taxPayerявляется атрибутом.
Адам Робинсон
2
Кроме того, это позволяет вам документировать функцию таким образом, чтобы ее можно было использовать, например, с помощью javadoc и быть публично видимым.
2
@dallin, Чистый код Боба Мартина показывает множество примеров полуреального кода. Если у вас слишком много функций в одном классе, возможно, ваш класс слишком большой?
Петер Тёрёк
67

Да, это может быть использовано для удовлетворения передового опыта. Например, лучше иметь функцию с четким именем, выполняющую некоторую работу, даже если длина ее составляет всего одну строку, чем иметь эту строку кода в более крупной функции и требовать однострочный комментарий, объясняющий, что она делает. Кроме того, соседние строки кода должны выполнять задачи на одном уровне абстракции. Контрпример был бы что-то вроде

startIgnition();
petrolFlag |= 0x006A;
engageChoke();

В этом случае определенно лучше переместить среднюю строку в функцию с разумным именем.

Килиан Фот
источник
15
Это 0x006A прекрасно: DA хорошо известна константа карбонизированного топлива с присадками a, b и c.
Кодер
2
+1 Я все для самодокументируемого кода :) кстати, я думаю, что я согласен на поддержание постоянного уровня абстракции через блоки, но я не мог объяснить почему. Не могли бы вы расширить это?
VEMV
3
Если вы просто говорите, что petrolFlag |= 0x006A;без какого-либо принятия решения, было бы лучше просто сказать petrolFlag |= A_B_C; без дополнительной функции. Предположительно, его engageChoke()следует вызывать только в том случае, если он petrolFlagсоответствует определенным критериям, и в нем должно быть четко сказано: «Мне нужна функция здесь». Просто небольшая гнида, этот ответ, в основном, точный, в противном случае :)
Тим Пост
9
+1 за указание на то, что код в методе должен быть на том же уровне абстракции. @vemv, это хорошая практика, потому что она облегчает понимание кода, позволяя вашему разуму не перепрыгивать между различными уровнями абстракции при чтении кода. Связывание переключений уровня абстракции с вызовами / возвратами методов (т. Е. Структурными «переходами») - хороший способ сделать код более плавным и чистым.
Петер Тёрёк
21
Нет, это полностью выдуманная ценность. Но тот факт, что вы задаетесь вопросом об этом, только подчеркивает: если бы код сказал mixLeadedGasoline(), вам бы не пришлось!
Килиан Фот
37

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

bool someConditionSatisfied = [complex expression];

Это даст подсказку читателю кода и избавит от введения новой функции.

cybevnm
источник
1
Bool особенно полезен, если имя функции условия будет трудным или, возможно, вводящим в заблуждение (например, IsEngineReadyUnlessItIsOffOrBusyOrOutOfService).
ДБКК
2
Я посчитал этот совет плохой идеей: бул и состояние отвлекают внимание от основной деятельности функции. Кроме того, стиль, предпочитающий локальные переменные, усложняет рефакторинг.
Волк
1
@ Волк ОТО, я предпочитаю это вызову функции, чтобы уменьшить «глубину абстракции» кода. IMO, переход в функцию - это больший переключатель контекста, чем прямой и прямой строки кода, особенно если он просто возвращает логическое значение.
Каче
@ Kache Я думаю, это зависит от того, кодируете ли вы объектно-ориентированный код или нет, в случае ООП использование функции-члена делает дизайн намного более гибким. Это действительно зависит от контекста ...
Вольф
23

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

Следуя примеру Петра, если это

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}

становится этим

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isMutant() 
        && (taxPayer.getNumberOfThumbs() > 2 
        || (taxPayer.getAge() > 123 && taxPayer.getEmployer().isXMan()));
}

Вы делаете одно редактирование, и оно обновляется повсеместно. Ремонтопригодность мудрая, это плюс.

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

Стивен
источник
2
Я уже знаю о преимуществах сохранения единой точки редактирования - вот почему вопрос гласит «... вызывается один раз». Тем не менее, приятно знать об этих оптимизациях компилятора, я всегда думал, что они следуют этим инструкциям буквально.
vemv
Разделение метода для проверки простого условия имеет тенденцию подразумевать, что условие может быть изменено путем изменения метода. Такое предположение полезно, когда это правда, но может быть опасно, когда это не так. Например, предположим, что код должен делить вещи на легкие объекты, тяжелые зеленые объекты и тяжелые не зеленые объекты. Объекты обладают быстрым свойством «казалось бы зеленым», которое надежно для тяжелых объектов, но может возвращать ложные срабатывания с легкими объектами; у них также есть функция «measureSpectralResponse», которая медленная, но будет надежно работать для всех объектов.
суперкат
Тот факт, что seemsGreenэто будет ненадежно с легкими объектами, не будет иметь значения, если код никогда не заботится о том, зеленые ли легкие объекты. Однако, если определение «облегченного» изменяется так, что некоторые не зеленые объекты, для которых возвращается значение true seemsGreen, не отображаются как «облегченные», то такое изменение в определении «облегченного» может нарушить код, который проверяет объекты быть "зеленым". В некоторых случаях совместное тестирование зеленого цвета и веса в коде может сделать отношения между тестами более ясными, чем если бы они были отдельными методами.
суперкат
5

В дополнение к удобочитаемости (или в дополнение к ней) это позволяет писать функции на должном уровне абстракции.

tylermac
источник
1
Боюсь, я не понимаю, что вы имели в виду ...
VEMV
1
@vemv: я думаю, что под «надлежащим уровнем абстракции» он подразумевает, что вы не получите код, в котором смешаны разные уровни абстракции (то есть то, что уже сказал Килиан Фот). Вы не хотите, чтобы ваш код обрабатывал, казалось бы, незначительные детали (например, образование всех связей в топливе), когда окружающий код рассматривает более высокоуровневое представление о ситуации под рукой (например, запуск двигателя). Первое загромождает второе и делает ваш код менее легким для чтения и сопровождения, так как вам придется учитывать все уровни абстракции одновременно в любое время.
Egon
4

По-разному. Иногда лучше инкапсулировать выражение в функцию / метод, даже если это всего одна строка. Если читать сложно или вам это нужно в нескольких местах, тогда я считаю это хорошей практикой. В долгосрочной перспективе его легче поддерживать, поскольку вы ввели единую точку изменений и улучшенную читаемость .

Однако иногда это просто то, что вам не нужно. Если выражение легко читается и / или просто появляется в одном месте, не оборачивайте его.

сокол
источник
3

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

WojonsTech
источник
Согласно ответу Стивена, это не всегда может случиться (хотя слепое доверие к магии компиляторов не может быть хорошим в любом случае)
vemv
1
да, это должно быть выяснено в зависимости от множества фактов. Если это язык с двойным интерфейсом, система будет использовать функции с одной строкой каждый раз, если вы не установите что-то для кеша, что может не сработать. Теперь, когда речь заходит о компиляторах, в день слабости и умиротворения это имеет значение, если компилятор станет вашим другом и уберет ваш маленький беспорядок или если он подумает, что вам это действительно нужно. Я помню, что если вы точно знаете, сколько раз цикл всегда будет выполняться, лучше просто скопировать и вставить его столько раз, чем выполнить цикл.
WojonsTech
+1 за выравнивание планет :) но твое последнее предложение звучит для меня совершенно чокнутым, ты действительно так делаешь?
VEMV
Это действительно зависит в большинстве случаев, нет, если я не получу правильную сумму платежа, чтобы проверить, действительно ли он увеличивает скорость и тому подобное. Но в старых компиляторах было бы лучше скопировать и вставить его, чем оставить компилятор, чтобы выяснить это 9/10.
WojonsTech
3

Я сделал это точно недавно в приложении, которое я рефакторинг, чтобы сделать явный смысл кода без комментариев:

protected void PaymentButton_Click(object sender, EventArgs e)
    Func<bool> HaveError = () => lblCreditCardError.Text == string.Empty && lblDisclaimer.Text == string.Empty;

    CheckInputs();

    if(HaveError())
        return;

    ...
}
joshperry
источник
Просто любопытно, что это за язык?
августа
5
@vemv: для меня это похоже на C #.
Скотт Митчелл
Я также предпочитаю дополнительные идентификаторы над комментариями. Но действительно ли это отличается от введения локальной переменной, чтобы сохранить ifкороткую позицию? Этот локальный (лямбда) подход делает PaymentButton_Clickфункцию (в целом) труднее для чтения. lblCreditCardErrorВ вашем примере , как представляется, является членом, так и HaveErrorесть (частный) предикат , который действителен для объекта. Я бы склонялся к тому, чтобы понизить это, но я не программист на C #, поэтому я сопротивляюсь.
Волк
@ Волк Эй, чувак, да. Я написал это довольно давно :) Я определенно делаю вещи совсем по-другому в наши дни. На самом деле, просмотр содержимого ярлыка, чтобы увидеть, произошла ли ошибка, заставляет меня съеживаться ... Почему я просто не вернул bool from CheckInputs()???
Joshperry
0

Перемещение этой строки в метод с именем с именем облегчает чтение вашего кода. Многие другие уже упоминали это («самодокументируемый код»). Другое преимущество переноса его в метод состоит в том, что он облегчает юнит-тестирование. Когда он изолирован в своем собственном методе и проверен модулем, вы можете быть уверены, что если / когда будет обнаружена ошибка, его не будет в этом методе.

Андрей
источник
0

Хороших ответов уже много, но стоит упомянуть особый случай .

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

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

def sophisticatedHello():
    # todo set up
    say("hello")
    # todo tear down

может также измениться в этом

def sophisticatedHello():
    setUp()
    say("hello")
    tearDown()

1) вы должны быть действительно уверены в этом (см. Принцип ЯГНИ )

волк
источник
0

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

someCondition = lambda p: True if [complicated expression involving p] else False
#I explicitly write the function with a ternary to make it clear this is a a predicate
if (someCondition(p)):
    #do stuff...

ИМХО, это хороший компромисс, потому что он дает вам удобство чтения, так как сложное выражение не загромождает ifусловие, избегая загромождения пространства имен global / package с маленькими одноразовыми метками. Дополнительным преимуществом является то, что функция «определение» находится там, где она используется, что упрощает изменение и чтение определения.

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

#goal - I have a list of PIL Image objects and I want them all as grayscale (uint8) numpy arrays
im_2_arr = lambda im: array(im.convert('L')) 
arr_list = [im_2_arr(image) for image in image_list]
crasic
источник
Почему «лямбда-р: истина, если [сложное выражение, включающее р], иначе ложь» вместо «лямбда-р: [сложное выражение, включающее р]»? 8-)
Ганс-Петер Стёрр,
@hstoerr в комментарии прямо под этой строкой. Я хотел бы четко заявить, что некоторые условия являются предикатом. Хотя это совершенно не нужно, я пишу множество научных сценариев, которые читают люди, которые не так много пишут, но лично я считаю более уместным иметь дополнительную краткость, а не путать моих коллег, потому что они не знают ни этого, [] == Falseни какого-либо другого похожая питоническая эквивалентность, которая не всегда интуитивна. Это в основном способ пометить, что someCondition на самом деле является предикатом.
авария
Просто для того, чтобы очистить мою очевидную ошибку, [] != Falseно []является ложным, когда приведен к
булле
@crasic: когда я не ожидаю, что мои читатели узнают, что [] оценивается как False, я предпочитаю делать len([complicated expression producing a list]) == 0, а не использовать, True if [blah] else Falseчто все еще требует, чтобы читатель знал, что [] оценивает как False.
Ли Райан