Чистый код: последствия коротких методов с несколькими параметрами

15

Недавно во время обзора кода я наткнулся на код, написанный новым коллегой, который содержит шаблон с запахом. Я подозреваю, что решения моего коллеги основаны на правилах, предложенных известной Книгой Чистого Кодекса (и, возможно, другими подобными книгами).

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

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

Я подозреваю, что этот код был вдохновлен советом, чтобы методы были короткими (<= 5 строк кода), чтобы избежать больших списков параметров (<3 параметра), и что конструкторы не должны выполнять работу (например, выполнять какие-то вычисления) это важно для законности объекта).

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

Тем не менее, я бы предпочел полностью изменить код, чтобы класс превратился в реальный объект, а не в серию методов, которые должны вызываться (процедурно) в определенном порядке.

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

user2180613
источник
6
1. На мгновение сыграть адвоката дьявола ... Работает ли код на самом деле? Потому что Объекты Передачи Данных - совершенно правильная техника, и если это все, что это ...
Роберт Харви,
7
2. Если вам не хватает слов для описания проблемы, значит, у вас недостаточно опыта, чтобы опровергнуть позицию вашего коллеги.
Роберт Харви
4
3. Если у вас есть рабочий код, который вы можете опубликовать, опубликуйте его в Code Review и дайте им возможность взглянуть на него. В противном случае это просто блуждающая общность.
Роберт Харви
5
@RobertHarvey «Значения, которые являются результатом вычислений, присваиваются свойствам, которые будут использоваться внутри нескольких частных методов в классе», для меня это не звучит как уважающий себя DTO. Я согласен, что немного больше конкретики было бы полезно.
topo Восстановить Монику
4
В сторону: Похоже, что кто-то на самом деле не читал Чистый код до того, как его избили. Я просто отсканировал его снова и не смог найти места, где он предлагает «конструкторы не должны выполнять работу» (некоторые примеры действительно выполняют работу), и предлагаемое решение для избежания слишком большого количества параметров состоит в создании объекта параметра, объединяющего связанные группы параметров, а не убивать ваши функции. И книга действительно предлагает рефакторинг кода, чтобы избежать временных зависимостей между методами. Я думаю, что твоя предвзятость по отношению к нескольким из его предпочтительных стилей кода окрашивала твое восприятие книги.
Эрик Кинг,

Ответы:

13

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

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

Чистый код предлагает, чтобы методы были короткими, с минимально возможным количеством аргументов. Но в соответствии с этими руководящими принципами предлагается, чтобы мы следовали принципам S OLID, повышали сплоченность и уменьшали сцепление .

S в ТВЕРДЫХ трибунами на единой ответственности принцип, который гласит , что объект должен нести ответственность только одну вещь. «Вещи» не очень точный термин, поэтому описания этого принципа сильно различаются. Тем не менее, дядя Боб, автор Чистого кода, также является человеком, который придумал этот принцип, описав его следующим образом: «Соберите вещи, которые меняются по одним и тем же причинам. Разделите вещи, которые меняются по разным причинам». Он продолжает говорить, что он имеет в виду с причинами, чтобы изменить здесь и здесь(более длинное объяснение здесь было бы слишком много). Если этот принцип был применен к классу, с которым вы имеете дело, весьма вероятно, что части, которые имеют дело с вычислениями, будут отделены от частей, которые имеют дело с состоянием удержания, путем разделения класса на два или более, в зависимости от того, сколько причин изменить эти расчеты есть.

Кроме того, классы Clean должны быть связными , что означает, что большинство его методов используют большинство своих атрибутов. Таким образом, максимально связный класс - это класс, в котором все методы используют все свои атрибуты; Например, в графическом приложении у вас может быть Vectorкласс с атрибутами Point aи Point b, где используются только методы, scaleBy(double factor)и printTo(Canvas canvas)оба работают с обоими атрибутами. Напротив, минимально связный класс - это класс, в котором каждый атрибут используется только в одном методе, и никогда не используется более одного атрибута для каждого метода. В среднем, класс представляет несвязную «группа» когезионных части - то есть несколько методов , используемых атрибуты a, bи c, в то время как использование отдыха cиd - это означает, что если мы разделим класс на две части, мы получим два связанных объекта.

Наконец, классы Clean должны максимально уменьшить сцепление . Несмотря на то, что здесь есть много типов связывания, которые стоит обсудить, кажется, что данный код в основном страдает от временного связывания , когда, как вы указали, методы объекта будут работать, как и ожидалось, только тогда, когда они вызываются в правильном порядке. И , как два выше рекомендации выше, решения этого обычно включают расщепление класса в двух или более сплоченных объектов. Стратегия разделения в этом случае обычно включает в себя шаблоны, такие как Builder или Factory, а в очень сложных случаях - State-Machines.

Рекомендации TL; DR: Чистый кодекс, которым следовал ваш коллега, хороши, но только в том случае, если они следуют остальным принципам, практикам и схемам, упомянутым в книге. Чистая версия «класс» Вы видите , будет разбиты на несколько классов, каждый с одной ответственностью, сплоченными методами и без временной связи. Это тот контекст, в котором небольшие методы и аргументы "мало-нет" имеют смысл.

MichelHenrich
источник
1
И вы, и Топо Морто написали хороший ответ, но я могу принять только один. Мне нравится, что вы обратились к SRP, связности и сцеплению. Это полезные термины, которые я могу использовать в обзоре кода. Разделение объекта на более мелкие объекты с их собственными обязанностями - это, очевидно, путь. Один (неконструктивный) метод, который инициализирует значения в кучу свойств класса, является мертвой раздачей, что новый объект должен быть возвращен. Я должен был это увидеть.
user2180613
1
SRP - это самое важное руководство; одно кольцо, чтобы управлять ими всеми и всем этим. Хорошая работа SRP приводит к более коротким методам. Пример: у меня есть фронтальный класс с 2 открытыми и около 8 закрытыми методами. Ни один не более чем ~ 3 строки; Весь класс составляет около 35 лок. Но я написал этот класс в прошлом! К тому времени, когда весь базовый код был написан, этот класс, по сути, написал сам, и я не должен был, на самом деле, не сделать методы больше. Я ни разу не сказал: «Я собираюсь написать эти методы в 5 строк, если это меня убьет». Каждый раз, когда вы применяете SRP, это просто происходит.
радаробоб
11

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

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

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

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

Подозреваю, что этот код был вдохновлен советом, чтобы методы были короткими (<= 5 строк кода), чтобы избежать больших списков параметров (<3 параметра), и что конструкторы не должны выполнять работу (например, выполнять вычисления некоторого вида, которые имеет важное значение для действительности объекта).

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

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

Тем не менее, я бы предпочел полностью изменить код, чтобы класс превратился в реальный объект, а не в серию методов, которые должны вызываться (процедурно) в определенном порядке.

Вы, вероятно, были бы правы сделать это. Нет ничего плохого в написании простого процедурного кода; Существует проблема в злоупотреблении парадигмой ОО для написания запутанного процедурного кода.

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

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

topo Восстановить Монику
источник
2
upvote потому что: 1. Подчеркивая SRP. «... маленькие методы ... следуют естественным образом» 2. Цель конструктора - допустимое состояние. 3. «Следовать некоторым руководствам по кодированию, игнорируя другие». Это ходячие мертвецы кодирования.
радаробоб
6

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

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

Карл Билефельдт
источник