Когда парадигма «Делай одно» становится вредной?

21

Ради аргумента вот пример функции, которая печатает содержимое данного файла построчно.

Версия 1:

void printFile(const string & filePath) {
  fstream file(filePath, ios::in);
  string line;
  while (std::getline(file, line)) {
    cout << line << endl;
  }
}

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

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

Версия 2:

void printFile(const string & filePath) {
  fstream file(filePath, ios::in);
  printLines(file);
}

void printLines(fstream & file) {
  string line;
  while (std::getline(file, line)) {
    printLine(line);
  }
}

void printLine(const string & line) {
  cout << line << endl;
}

Я понимаю, чего они хотят достичь (открыть файл / прочитать строки / напечатать строку), но не слишком ли это излишне?

Оригинальная версия проста и в некотором смысле уже делает одну вещь - печатает файл.

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

Не лучше ли в этом случае иметь код в одном месте?

В какой момент парадигма «Делай одно» становится вредной?

Petr
источник
13
Этот вид практики кодирования всегда основан на каждом конкретном случае. Там никогда не бывает единого подхода.
Iammilind
1
@ Алекс - Принятый ответ буквально не имеет ничего общего с вопросом. Я нахожу это действительно странным.
ChaosPandion
2
Я отмечаю, что ваша переработанная версия перевернута, что приводит к недостаточной читаемости. Чтение вниз файла, можно было бы ожидать , чтобы увидеть printFile, printLinesи , наконец printLine.
Энтони Пеграм
1
@Kev, я еще раз могу только не согласиться, особенно с этой категоризацией. Это не педантизм, это главное! Это ОП, который конкретно говорит, что вторая версия может быть не столь удобочитаемой. Это ОП, который специально цитирует Чистый код как вдохновение для второй версии. Мой комментарий, по сути, заключается в том, что Чистый код не позволил бы ему писать код таким образом. Порядок на самом деле важен для читабельности, вы читаете файл так же, как читаете газетную статью, получаете все больше и больше подробностей, пока в основном не становитесь незаинтересованными.
Энтони Пеграм
1
Подобно тому, как вы не ожидаете прочитать стихотворение задом наперед, так же как и не ожидаете увидеть самый низкий уровень детализации как первое, что есть внутри определенного класса. На ваш взгляд, этот код занимает мало времени для быстрой сортировки, но я бы только предположил, что этот код не единственный код, который он собирается написать. На мой взгляд, если он собирается ссылаться на Чистый код, по крайней мере, он может следовать ему. Если код не в порядке, он будет менее читабельным, чем в противном случае.
Энтони Пеграм

Ответы:

15

Конечно, это просто напрашивается вопрос : «Что такое одна вещь?» Чтение строки - это одно, а запись - другое? Или копирование строки из одного потока в другой считается одним? Или копирование файла?

На это нет твердого, объективного ответа. Тебе решать. Вы можете решить. Вы должны решить. Основная цель парадигмы «сделать что-то одно», вероятно, состоит в том, чтобы создать код, который будет настолько прост для понимания, насколько это возможно, поэтому вы можете использовать его в качестве руководства. К сожалению, это также не поддается объективной оценке, поэтому приходится полагаться на свое внутреннее чувство и "WTF?" посчитать в обзоре кода .

IMO функция, состоящая всего из одной строки кода, редко когда-либо стоит того. Ваш не printLine()имеет никакого преимущества перед использованием std::cout << line << '\n'1 напрямую. Если я вижу printLine(), я должен либо предположить, что он делает то, что говорит его название, либо посмотреть и проверить. Если я вижу std::cout << line << '\n', я сразу же знаю, что он делает, потому что это канонический способ вывода содержимого строки в виде строки std::cout.

Однако другой важной целью парадигмы является повторное использование кода, и это гораздо более объективная мера. Например, в вашей 2-й версии printLines() можно было бы легко написать так, чтобы это был универсально полезный алгоритм, который копирует строки из одного потока в другой:

void copyLines(std::istream& is, std::ostream& os)
{
  std::string line;
  while( std::getline(is, line) );
    os << line << '\n';
  }
}

Такой алгоритм можно использовать и в других контекстах.

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

void printFile(const std::string& filePath) {
  std::ifstream file(filePath.c_str());
  printLines(file, std::cout);
}

1 Обратите внимание, что я использовал, '\n'а не std::endl. '\n'должен быть выбором по умолчанию для вывода новой строки , std::endlэто нечетный случай .

SBI
источник
2
+1 - Я в основном согласен, но я думаю, что это нечто большее, чем «интуиция». Беда в том, что люди судят об «одном», подсчитывая детали реализации. Для меня функция должна реализовывать (и ее название описывать) единственную понятную абстракцию. Вы никогда не должны называть функцию "do_x_and_y". Реализация может и должна делать несколько (более простых) вещей - и каждая из этих более простых вещей может быть разложена на несколько даже более простых вещей и так далее. Это просто функциональная декомпозиция с дополнительным правилом - каждая из функций (и их имена) должна описывать одну четкую концепцию / задачу / что угодно.
Steve314
@ Steve314: я не перечислял детали реализации как возможности. Копирование строк из одного потока в другой - однозначная абстракция. Или это? И этого легко избежать do_x_and_y(), назвав do_everything()вместо этого функцию . Да, это глупый пример, но он показывает, что это правило не может даже предотвратить самые крайние примеры плохого дизайна. IMO это является решением чувства кишки столько , сколько один диктуемых конвенциями. В противном случае, если бы это было объективно, вы могли бы придумать метрику для этого - что вы не можете.
ВОО
1
Я не собирался противоречить - только предложить дополнение. Я предполагаю, что я забыл сказать, что, исходя из вопроса, разложение на и printLineт. Д. Является допустимым - каждый из них является отдельной абстракцией - но это не означает, что это необходимо. printFileэто уже "одна вещь". Хотя вы можете разложить это на три отдельные абстракции нижнего уровня, вам не нужно разлагать на каждом возможном уровне абстракции. Каждая функция должна выполнять «одну вещь», но не каждая возможная «одна вещь» должна быть функцией. Перемещение слишком большой сложности в граф вызовов может само по себе стать проблемой.
Steve314
7

Выполнение функции «только одно» - это средство для достижения двух желаемых целей, а не заповедь от Бога:

  1. Если ваша функция выполняет только «одну вещь», это поможет вам избежать дублирования кода и раздувания API, потому что вы сможете составлять функции для выполнения более сложных задач вместо использования комбинаторного взрыва высокоуровневых, менее компонуемых функций ,

  2. Наличие функций, выполняющих только «одну вещь», может сделать код более читабельным. Это зависит от того, достигаете ли вы большей ясности и простоты рассуждения, отделяя вещи, чем теряете многословность, косвенность и концептуальные издержки конструкций, которые позволяют вам отделить вещи.

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

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

dsimcha
источник
Я бы добавил третье, и это меньше, функции легче тестируются. Поскольку, вероятно, требуется меньше входных данных, если функция выполняет только одно действие, это облегчает независимое тестирование.
PersonalNexus
@PersonalNexus: Я несколько согласен с вопросом тестирования, но, имхо, глупо тестировать детали реализации. Для меня модульный тест должен проверить «одну вещь», как это определено в моем ответе. Все, что более мелкозернисто, делает ваши тесты хрупкими (потому что изменение деталей реализации потребует изменения ваших тестов), а ваш код раздражающе многословен, косвенен и т. Д. (Потому что вы будете добавлять косвенные ссылки только для поддержки тестирования).
dsimcha
6

В этом масштабе это не имеет значения. Однофункциональная реализация совершенно очевидна и понятна. Однако добавление немного большей сложности делает очень привлекательным отделение итерации от действия. Например, предположим, что вам нужно напечатать строки из набора файлов, указанных шаблоном, например «* .txt». Тогда я бы отделил итерацию от действия:

printLines(FileSet files) {
   files.each({ 
       file -> file.eachLine({ 
           line -> printLine(line); 
       })
   })
}

Теперь итерация файла может быть проверена отдельно.

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

Кевин Клайн
источник
4
Я думаю, что ты прибил это. Если нам нужен комментарий для объяснения строки, тогда всегда пора извлечь метод.
Роджер КС Вернерссон
5

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

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

Роджер К.С. Вернерссон
источник
3

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

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

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

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

Майкл Браун
источник
2
Я проголосовал за этот ответ, потому что мне нравится логика, хотя я с ней не согласен! Обеспечить структуру, основанную на некотором сложном осмыслении того, что может произойти в будущем, контрпродуктивно. Факторизовать код, когда вам нужно. Не абстрагируйся, пока не понадобится. Современный код страдает от людей, пытающихся рабски следовать правилам вместо того, чтобы просто писать работающий код и неохотно его адаптировать . Хорошие программисты ленивы .
Иттрилл
Спасибо за комментарий. Заметьте, я не защищаю преждевременное абстрагирование, а просто делю логические операции, чтобы потом было легче это сделать.
Майкл Браун
2

Я лично предпочитаю последний подход, потому что он спасает вашу работу в будущем и заставляет думать, «как это сделать общим образом». Несмотря на то, что в вашем случае Версия 1 лучше, чем Версия 2 - только потому, что проблемы, решаемые Версией 2, слишком тривиальны и специфичны для fstream. Я думаю, что это должно быть сделано следующим образом (включая исправление ошибок, предложенное Nawaz):

Общие служебные функции:

void printLine(ostream& output, const string & line) { 
    output << line << endl; 
} 

void printLines(istream& input, ostream& output) { 
    string line; 
    while (getline(input, line)) {
        printLine(output, line); 
    } 
} 

Доменная функция:

void printFile(const string & filePath, ostream& output = std::cout) { 
    fstream file(filePath, ios::in); 
    printLines(file, output); 
} 

Теперь printLinesи printLineможно работать не только fstreamс любым, но и с любым потоком.

gwiazdorrr
источник
2
Я не согласен. Эта printLine()функция не имеет значения. Смотри мой ответ .
СБИ
1
Хорошо, если мы сохраним printLine (), тогда мы можем добавить декоратор, который добавляет номера строк или синтаксическую раскраску. Сказав это, я не буду извлекать эти методы, пока не найду причину.
Роджер КС Вернерссон
2

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

Таким образом, правильный ответ на вопрос требует хорошей способности «предвидеть» будущее, например:

  • Я сейчас обязан сделать AиB
  • Какова вероятность того, что в ближайшем будущем я также должен буду сделать A-и B+(то есть что-то похожее на А и В, но немного отличающееся)?
  • Какова вероятность того, что A + станет A*или в более отдаленном будущем A*-?

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

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

Просто в качестве примера, позвольте мне рассказать вам эту реальную историю:

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

После некоторого исследования я обнаружил, что, будучи частой проблемой, все студенты приходят к мысли использовать функцию для этого. После того, как они сказали им, что есть функция библиотеки для этого ( strlen), многие из них ответили, что, поскольку проблема была настолько простой и тривиальной, им было бы эффективнее написать свою собственную функцию (2 строки кода), чем искать руководство по библиотеке C (это был 1984 год, забыл WEB и Google!) в строгом алфавитном порядке, чтобы увидеть, есть ли готовая функция для этого.

Это пример, когда парадигма «не изобретать велосипед» может стать вредной без эффективного каталога колес!

Эмилио Гаравалья
источник
2

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

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

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

Безопасный
источник
2

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

Из оригинального поста

void printLine(const string & line) {
  cout << line << endl;
}

Если вы достаточно педантичны, вы можете заметить, что printLine по-прежнему делает две вещи: записывает строку в cout и добавляет символ «конец строки». Некоторые люди могут захотеть справиться с этим путем создания новых функций:

void printLine(const string & line) {
  reallyPrintLine(line);
  addEndLine();
}

void reallyPrintLine(const string & line) {
  cout << line;
}

void addEndLine() {
  cout << endl;
}

О нет, теперь мы сделали проблему еще хуже! Теперь даже ЯВНО, что printLine делает ДВЕ вещи !!! 1! Не глупо создавать самые абсурдные «обходные пути», которые можно себе представить, просто чтобы избавиться от этой неизбежной проблемы, заключающейся в том, что печать строки состоит из печати самой строки и добавления символа конца строки.

void printLine(const string & line) {
  for (int i=0; i<2; i++)
    reallyPrintLine(line, i);
}

void reallyPrintLine(const string & line, int action) {
  cout << (action==0?line:endl);
}
user281377
источник
1

Короткий ответ ... это зависит.

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

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

Однако если вы уверены, что вам нужен только вывод в консоли, это не имеет особого смысла. Написание «обертки» cout <<кажется бесполезным.

Лучиан Григоре
источник
1
Но, строго говоря, разве функция printLine не отличается от уровня итерации по строкам?
@ Петр Я так думаю, поэтому они предлагают вам разделить функциональность. Я думаю, что концепция верна, но вы должны применять ее на индивидуальной основе.
1

Вся причина в том, что есть книги, посвящающие главы достоинствам «делай одно», состоит в том, что все еще есть разработчики, которые пишут функции длиной 4 страницы и вкладывают условные 6 уровней. Если ваш код прост и понятен, вы все сделали правильно.

Kevin
источник
0

Как прокомментировали другие авторы, выполнение одной вещи - это вопрос масштаба.

Я бы также предположил, что идея One Thing заключается в том, чтобы остановить людей, кодирующих побочные эффекты. Это иллюстрируется последовательной связью, где методы должны вызываться в определенном порядке, чтобы получить «правильный» результат.

NWS
источник