SOLID против статических методов

11

Вот проблема, с которой я часто сталкиваюсь: пусть будет проект интернет-магазина с классом Product. Я хочу добавить функцию, которая позволяет пользователям публиковать отзывы о продукте. Итак, у меня есть класс Review, который ссылается на продукт. Теперь мне нужен метод, который перечисляет все отзывы о продукте. Есть две возможности:

(А)

public class Product {
  ...
  public Collection<Review> getReviews() {...}
}

(В)

public class Review {
  ...
  static public Collection<Review> forProduct( Product product ) {...}
}

Посмотрев на код, я бы выбрал (A): он не статичен и не нуждается в параметре. Тем не менее, я чувствую, что (A) нарушает принцип единой ответственности (SRP) и принцип открытого и закрытого доступа (OCP), тогда как (B) не:

  • (SRP) Когда я хочу изменить способ сбора отзывов о продукте, я должен изменить класс продукта. Но причина изменения класса Product должна быть только одна. И это, конечно, не отзывы. Если я упакую каждую функцию, которая имеет какое-либо отношение к продуктам в Продукте, она скоро будет засорена.

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

Что важнее: следовать принципам SOLID или иметь более простой интерфейс?

Или я вообще что-то здесь не так делаю?

Результат

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

Позвольте мне обобщить основные аргументы из ответов:

  • pro (A): OCP не является законом, и читабельность кода также имеет значение.
  • pro (A): отношения объекта должны быть судоходными. Оба класса могут знать об этих отношениях.
  • pro (A) + (B): выполните оба действия и делегируйте их в (A) - (B), поэтому вероятность повторного изменения Product меньше.
  • pro (C): поместить методы поиска в третий класс (сервис), где он не является статичным.
  • Противоположность (B): препятствует насмешкам в тестах.

Несколько дополнительных вещей, которые сделали мои коллеги на работе:

  • pro (B): наша платформа ORM может автоматически генерировать код для (B).
  • pro (A): по техническим причинам нашей платформы ORM в некоторых случаях необходимо будет изменить «закрытую» сущность, независимо от того, куда идет поиск. Так что я не всегда смогу придерживаться SOLID.
  • Против (С): много суеты ;-)

Вывод

Я использую оба (A) + (B) с делегированием для моего текущего проекта. Однако в сервис-ориентированной среде я пойду с (C).

Wolfgang
источник
2
Пока это не статическая переменная, все круто. Статические методы просты в тестировании и прослеживаются.
Кодер
3
Почему бы просто не иметь класс ProductsReviews? Тогда Product и Review остаются прежними. Или, может быть, я неправильно понимаю.
ElGringoGrande
2
@ Кодер "Статические методы просто проверить", правда? Они не могут быть осмеяны, см .: googletesting.blogspot.com/2008/12/… для получения дополнительной информации.
StuperUser
1
@StuperUser: Нечего издеваться. Assert(5 = Math.Abs(-5));
Кодер
2
Тестирование Abs()не проблема, тестирование того, что от него зависит. У вас нет шва для изоляции зависимого тестируемого кода (CUT) для использования макета. Это означает, что вы не можете проверить его как элементарный модуль, и все ваши тесты станут интеграционными тестами, которые проверяют логику модуля. Сбой в тесте может быть в CUT или в Abs()(или его зависимом коде) и устраняет преимущества диагностики модульных тестов.
StuperUser

Ответы:

7

Что важнее: следовать принципам SOLID или иметь более простой интерфейс?

Интерфейс по отношению к SOLID

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

Открытый / Закрытый Принцип

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

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

Сделайте все возможное, у нас есть команда врачей

Если ваш анализ требований говорит о том, что вам необходимо выразить отношение Product-Review с обеих сторон, сделайте это.

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

radarbob
источник
1
+1 за то, что отметили, что OCP - это скорее показатель хорошего качества, а не быстрое правило для любого кода. Если вы обнаружите, что трудно или невозможно правильно следовать OCP, это признак того, что ваша модель нуждается в рефакторинге, чтобы обеспечить более гибкое повторное использование.
CodexArcanum
Я выбрал пример Product и Review, чтобы указать, что Product является основной сущностью проекта, тогда как Review - это просто дополнение. Таким образом, продукт уже существует, завершен, а проверка наступает позже и должна быть введена без открытия существующего кода (включая продукт).
Вольфганг
10

ТВЕРДЫЕ - это руководящие принципы, поэтому влияйте на принятие решений, а не диктуйте их.

При использовании статических методов следует учитывать их влияние на тестируемость .


Тестирование forProduct(Product product)не будет проблемой.

Будет проверено что-то, что от этого зависит.

У вас не будет шва для изоляции зависимого Code-Under-Test (CUT) для использования макета, поскольку при запуске приложения статические методы обязательно существуют.

Давайте иметь метод, CUT()который вызываетforProduct()

Если forProduct()это staticвы не можете проверить , CUT()как атомная единица и все тесты становятся интеграционные тесты , которые проверяют блок логики.

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

См. Этот отличный пост в блоге для получения более подробной информации: http://googletesting.blogspot.com/2008/12/static-methods-are-death-to-testability.html


Это может привести к разочарованию неудачными тестами и отказом от хороших практик и выгод, которые их окружают.

StuperUser
источник
1
Это очень хороший момент. В общем, не для меня, хотя. ;-) Я не настолько строг в написании модульных тестов, которые охватывают более одного класса. Они все идут в базу данных, это нормально для меня. Поэтому я не буду издеваться над бизнес-объектом или его поиском.
Вольфганг
+1, спасибо за установку красного флага для тестов! Я согласен с @Wolfgang, обычно я тоже не такой строгий, но когда мне нужны такие тесты, я действительно ненавижу статические методы. Обычно, если статический метод слишком сильно взаимодействует со своими параметрами или если взаимодействует с любым другим статическим методом, я предпочитаю делать его методом экземпляра.
Адриано Репетти
Разве это не зависит полностью от используемого языка? ОП использовал пример Java, но никогда не упоминал язык в вопросе, а также не указан в тегах.
Изката
1
@StuperUser If forProduct() is static you can't test CUT() as an atomic unit and all of your tests become integration tests that test unit logic.- Я полагаю, что Javascript и Python позволяют статическим методам быть переопределенными. Хотя я не уверен на 100%.
Изката
1
@Izkata JS динамически типизирован, поэтому не имеет static, вы имитируете его с помощью замыкания и шаблона синглтона. Читая на Python (особенно stackoverflow.com/questions/893015/… ), вы должны наследовать и расширять. Переопределение не является насмешкой; Похоже, у вас все еще нет шва для тестирования кода как атомарного модуля.
StuperUser
4

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

Например, я хотел бы ввести что-то, что сыграло роль ретривера. Я бы, наверное, дал ему интерфейс IRetrieveReviews. Вы можете поместить это в конструктор продукта (Dependency Injection). Если вы хотите изменить способ получения отзывов, вы можете сделать это легко, введя другого соавтора - a TwitterReviewRetrieverили a AmazonReviewRetrieverили MultipleSourceReviewRetrieverили все, что вам нужно.

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

Lunivore
источник
Походит на образец DAO, который очень распространен в сервисно-ориентированном программном обеспечении. Мне нравится эта идея, потому что она выражает то, что получение объектов не является ответственностью этих объектов. Тем не менее, поскольку я предпочитаю объектно-ориентированный способ, а не сервис-ориентированный.
Вольфганг
1
Использование подобного интерфейса IRetrieveReviewsперестает быть сервис-ориентированным - оно не определяет, что получает отзывы, как, и когда. Может быть, это сервис с множеством методов для подобных вещей. Может быть, это класс, который делает это одно. Может быть, это хранилище или HTTP-запрос к серверу. Вы не знаете Ты не должен знать. В этом-то и дело.
Lunivore
Да, так что это будет реализация стратегии. Что будет аргументом для третьего класса. (A) и (B) не поддержали бы это. Искатель обязательно будет использовать ORM, поэтому нет причин для замены алгоритма. Извините, если мне неясно об этом в моем вопросе.
Вольфганг
3

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

Это не правильно.

Вы помещаете статический метод в класс Review, потому что ... почему? Разве это не то, с чем вы боретесь? Разве это не вся проблема?

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

ElGringoGrande
источник
Я не согласен с тем, что использование метода на пересмотре нарушит SRP. По продукту, но не по обзору. Моя проблема была в том, что это статично. Если бы я переместил метод в какой-то третий класс, он все равно был бы статическим и имел бы параметр продукта.
Вольфганг
Итак, вы говорите, что единственная ответственность за класс Review, единственная причина его изменения, заключается в том, нужно ли менять статический метод forProduct? В классе Review нет других функций?
ElGringoGrande
Есть много вещей на обзор. Но я думаю, что forProduct (продукт) хорошо подошел бы к нему. Поиск Обзоров во многом зависит от атрибутов и структуры Обзора (какие уникальные идентификаторы, какие области видимости меняют атрибуты?). Продукт, однако, не знает и не должен знать об отзывах.
Вольфганг
3

Я бы не стал использовать функциональность «Получить отзывы о продукте» ни в Productклассе, ни в Reviewклассе ...

У вас есть место, где вы получаете ваши продукты, верно? Что-то с, GetProductById(int productId)а может GetProductsByCategory(int categoryId)и так далее.

Точно так же у вас должно быть место для получения ваших отзывов, с GetReviewbyId(int reviewId)и, возможно, с GetReviewsForProduct(int productId).

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

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

Эрик Кинг
источник
Вот как я справлюсь с этим, и меня смущает (и беспокоит, правильны ли мои собственные идеи) отсутствие этого ответа до вашего поста. Ясно, что ни класс, представляющий отчет, ни представление продукта, не должен нести ответственность за фактическое извлечение другого. Некоторые виды услуг по предоставлению данных должны справляться с этим.
CodexArcanum
@CodexArcanum Ну, вы не одиноки. :-)
Эрик Кинг,
2

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

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

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

Адриано Репетти
источник
Я также думал об этой идее о семантике обоих концов и о том, какая из них «более сильная», так что это потребовало бы поиска. Вот почему я придумал (B). Это также помогло бы создать иерархию «абстракции» бизнес-классов. Продукт был простым базовым объектом, где обзор - более высокий. Таким образом, обзор может ссылаться на продукт, но не наоборот. Таким образом можно избежать циклов ссылок между бизнес-классами, что станет еще одной проблемой в моем списке.
Вольфганг
Я думаю, что для небольшого набора классов было бы неплохо даже предоставить ОБА методы (A) и (B). Слишком часто пользовательский интерфейс недостаточно известен на момент написания этой логики.
Адриано Репетти
1

Ваш продукт может просто делегировать ваш статический метод Review, и в этом случае вы предоставляете удобный интерфейс в естественном месте (Product.getReviews), но детали вашей реализации находятся в Review.getForProduct.

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

pfries
источник
Я не уверен, что хотел бы этого. Таким образом, у меня есть метод в обоих местах, и это хорошо, потому что другим разработчикам не нужно искать в обоих классах, чтобы увидеть, где я его положил. С другой стороны, у меня были бы как недостатки статического метода, так и изменение «закрытого» класса Product. Я не уверен, что делегация помогает решить проблему трения. Если когда-либо была причина изменить искатель, это наверняка приведет к изменению подписи и, следовательно, к изменению продукта снова.
Вольфганг
Ключ OCP в том, что вы можете заменить реализации без изменения вашего кода. На практике это тесно связано с заменой Лискова. Одна реализация должна заменять другую. Вы можете иметь DatabaseReviews и SoapReviews как разные классы, реализующие IGetReviews.getReviews и getReviewsForProducts. Тогда ваша система будет открыта для расширения (изменение способа получения отзывов) и закрыта для модификации (зависимости от IGetReviews не нарушаются). Вот что я имею в виду под управлением зависимостями. В вашем случае вам придется изменить свой код, чтобы изменить его поведение.
pfries
Разве это не тот Принцип инверсии зависимости (DIP), который вы описываете?
Вольфганг
Это связанные принципы. Ваша точка замещения достигается с помощью DIP.
pfries
1

У меня немного другое мнение по этому поводу, чем у большинства других ответов. Я думаю, что Productи Reviewв основном это объекты передачи данных (DTO). В своем коде я стараюсь, чтобы мои DTO / Entities избегали поведения. Это просто хороший API для хранения текущего состояния моей модели.

Когда вы говорите о OO и SOLID, вы обычно говорите о «объекте», который не представляет состояние (обязательно), но вместо этого представляет какой-то сервис, который отвечает на вопросы для вас, или которому вы можете делегировать часть своей работы. , Например:

interface IProductRepository
{
    void SaveNewProduct(IProduct product);
    IProduct GetProductById(ProductId productId);
    bool TryGetProductByName(string name, out IProduct product);
}

interface IProduct
{
    ProductId Id { get; }
    string Name { get; }
}

class ExistingProduct : IProduct
{
    public ProductId Id { get; private set; }
    public string Name { get; private set; }
}

Тогда ваш фактический ProductRepositoryбудет возвращать ExistingProductдля GetProductByProductIdметода и т. Д.

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

Если вы измените схему базы данных, вы можете изменить реализацию своего хранилища, не меняя DTO и т. Д.

Короче, думаю, я бы не выбрал ни один из ваших вариантов. :)

Скотт Уитлок
источник
0

Статические методы могут быть сложнее тестировать, но это не значит, что вы не можете их использовать - вам просто нужно настроить их так, чтобы вам не нужно было их тестировать.

Сделайте оба product.GetReviews и Review.ForProduct однострочными методами,

new ReviewService().GetReviews(productID);

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

Если вы должны иметь 100% охват, попросите интеграционные тесты вызвать методы класса продукта / обзора.

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

Том Кларксон
источник