Применимость принципа единой ответственности

Недавно я столкнулся с кажущейся тривиальной архитектурной проблемой. У меня был простой репозиторий в моем коде, который назывался так (код на C #):

var user = /* create user somehow */;
_userRepository.Add(user);
/* do some other stuff*/
_userRepository.SaveChanges();

SaveChanges была простая оболочка, которая фиксирует изменения в базе данных:

void SaveChanges()
{
    _dataContext.SaveChanges();
    _logger.Log("User DB updated: " + someImportantInfo);
}

Затем, через некоторое время, мне нужно было реализовать новую логику, которая будет отправлять уведомления по электронной почте каждый раз, когда пользователь создается в системе. Поскольку было много обращений к системе _userRepository.Add()и SaveChangesвокруг нее , я решил обновить ее SaveChangesследующим образом:

void SaveChanges()
{
    _dataContext.SaveChanges();
    _logger.Log("User DB updated: " + someImportantInfo);
    foreach (var newUser in dataContext.GetAddedUsers())
    {
       _eventService.RaiseEvent(new UserCreatedEvent(newUser ))
    }
}

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

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

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

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

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

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

// In EventFiringUserRepo:
public void SaveChanges()
{
  _basicRepo.SaveChanges();
   FireEventsForNewlyAddedUsers();
}

private void FireEventsForNewlyAddedUsers()
{
  foreach (var newUser in _basicRepo.DataContext.GetAddedUsers())
  {
     _eventService.RaiseEvent(new UserCreatedEvent(newUser))
  }
}

Вы можете вызвать прокси-класс a NotifyingRepositoryили, ObservableRepositoryесли хотите, в соответствии с высоко оцененным ответом @ Peter (который на самом деле не говорит, как устранить нарушение SRP, а только говорит, что нарушение допустимо).

Новый и старый класс репозитория должны происходить из общего интерфейса, как показано в описании классического шаблона Proxy .

Затем в исходном коде инициализируйте _userRepositoryобъект нового EventFiringUserRepoкласса. Таким образом, вы сохраняете исходное хранилище отдельно от механики событий. При необходимости вы можете иметь хранилище событий и исходное хранилище рядом друг с другом и позволить вызывающим абонентам решать, используют ли они первый или второй.

Чтобы обратить внимание на одну проблему, упомянутую в комментариях: не приводит ли это к прокси поверх прокси поверх прокси и так далее? На самом деле, добавление механики событий создает основу для добавления дополнительных требований типа «отправка электронных писем», просто подписываясь на события, так что придерживаясь SRP с этими требованиями, без каких-либо дополнительных прокси. Но одна вещь, которая должна быть добавлена ​​однажды, это сама механика событий.

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

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

Конечно, вы не должны рассматривать Add как особый случай - вам придется запускать события для Modify и Delete также. Это особый подход к случаю «Добавить», который пахнет, заставляет читателя объяснить, почему он пахнет, и в конечном итоге заставляет некоторых читателей кода прийти к выводу, что он должен нарушать SRP.

«Уведомляющий» репозиторий, который можно запрашивать, изменять и генерировать события при изменениях, является совершенно нормальным объектом. Можно ожидать, что вы найдете несколько их вариантов практически в любом приличном проекте.

Но действительно ли вам нужен «уведомляющий» репозиторий? Вы упомянули C #: Многие люди согласятся с тем, что использование System.Collections.ObjectModel.ObservableCollection<>вместо того, System.Collections.Generic.List<>когда последнее - это все, что вам нужно, это все виды плохого и неправильного, но немногие сразу указывают на SRP.

То, что вы делаете сейчас, это обменять вас UserList _userRepositoryс ObservableUserCollection _userRepository. Если это лучший курс действий или нет, зависит от приложения. Но, несмотря на то, что он, несомненно, делает его _userRepositoryзначительно менее легким, по моему скромному мнению, он не нарушает SRP.

Да, это нарушение принципа единой ответственности и действительного положения.

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

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

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

Вероятно, стоит остановиться на комментариях ниже.

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

Неправильно. Вы объединяете «Добавлено в репозиторий» и «Новое».

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

«Новый» - это состояние пользователя, определяемое бизнес-правилами.

В настоящее время бизнес-правило может быть «Новое == только что добавлено в репозиторий», но это не означает, что знать и применять это правило не является отдельной обязанностью.

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

ИМХО в этом ответе совершенно не хватает смысла: репо - это единственное центральное место в коде, которое знает, когда добавляются новые пользователи.

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

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

Это действительная точка зрения?

Да, это так, хотя это сильно зависит от структуры вашего кода. У меня нет полного контекста, поэтому я постараюсь говорить в целом.

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

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

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

SRP работает в тандеме с ISP (S и я в SOLID). В итоге вы получите множество классов и методов, которые делают очень специфические вещи и ничего больше. Они очень сфокусированы, их очень легко обновлять или заменять, и, как правило, их легко тестировать. Конечно, на практике у вас также будет несколько больших классов, которые занимаются оркестровкой: они будут иметь ряд зависимостей и будут сосредоточены не на отдельных действиях, а на деловых действиях, которые могут потребовать нескольких шагов. Пока бизнес-контекст понятен, их тоже можно назвать единственной ответственностью, но, как вы правильно сказали, по мере роста кода вы можете захотеть абстрагировать некоторые из них в новые классы / интерфейсы.

Теперь вернемся к вашему конкретному примеру. Если вам абсолютно необходимо отправлять уведомление всякий раз, когда пользователь создается, и, возможно, даже выполнять другие, более специализированные действия, тогда вы можете создать отдельный сервис, который инкапсулирует это требование, что-то вроде UserCreationService, который предоставляет один метод Add(user), который обрабатывает как хранилище (вызов в ваш репозиторий) и уведомление как единое деловое действие. Или сделайте это в своем оригинальном фрагменте, после_userRepository.SaveChanges();

SRP, теоретически, о людях , как объясняет дядя Боб в своей статье «Принцип единой ответственности» . _{Спасибо Роберту Харви за то, что предоставили это в своем комментарии.}

Правильный вопрос:

Какая заинтересованная сторона добавила требование "отправлять электронные письма"?

Если этот участник также отвечает за сохранение данных (маловероятно, но возможно), это не нарушает ПСП. В противном случае это так.

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

Создание пользователя, определение нового пользователя и его постоянство - это 3 разные вещи .

^{Помещение шахты}

Примите во внимание предыдущую предпосылку, прежде чем решить, является ли хранилище подходящим местом для уведомления о бизнес-событиях (независимо от SRP). Обратите внимание, что я сказал бизнес-событие, потому что для меня UserCreatedимеет значение, отличное от UserStoredили UserAdded ¹ . Я также считаю, что каждое из этих событий должно быть адресовано разной аудитории.

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

С другой стороны, это не обязательно верно, что _dataContext.SaveChanges();пользователь успешно сохранился. Это будет зависеть от объема транзакций базы данных. Например, это может быть верно для баз данных, таких как MongoDB, где транзакции являются атомарными, но не может быть, для традиционных СУБД, реализующих транзакции ACID, где может быть больше транзакций, которые еще предстоит зафиксировать.

Это действительная точка зрения?

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

• Удобно ли запускать бизнес-события из компонентов, не подозревающих о текущих бизнес-операциях?
• Они представляют правильное место столько, сколько подходящий момент, чтобы сделать это?
• Должен ли я позволить этим компонентам управлять моей бизнес-логикой с помощью таких уведомлений?
• Могу ли я аннулировать побочные эффекты, вызванные преждевременными событиями? ²

Мне кажется, что поднятие события здесь - это то же самое, что и регистрация

Точно нет. Однако ведение журнала не должно иметь побочных эффектов, так как вы предположили, что событие UserCreatedможет вызвать другие бизнес-операции. Нравится уведомления. ³

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

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

^{1: Именование вещей адекватно также имеет значение.}

^{2: скажем, мы отправили UserCreatedпосле _dataContext.SaveChanges();, но вся транзакция с базой данных не удалась позже из-за проблем с соединением или нарушений ограничений Будьте осторожны с преждевременной трансляцией событий, потому что ее побочные эффекты трудно отменить (если это вообще возможно).}

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

Нет, это не нарушает ПСП.

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

Но это не то, что означает принцип. Речь идет о проблемах бизнес-уровня. Класс не должен реализовывать множественные проблемы или требования, которые могут изменяться независимо на уровне бизнеса. Допустим, класс хранит пользователя и отправляет приветственное сообщение с жестким кодом по электронной почте. Многочисленные независимые проблемы могут привести к изменению требований такого класса. Дизайнеру может потребоваться изменить html / stylesheet почты. Эксперт по коммуникациям может потребовать изменения формулировки письма. И эксперт по UX мог бы решить, что почта должна быть отправлена ​​в другой точке входящего потока. Таким образом, класс подвергается множественным изменениям требований из независимых источников. Это нарушает ПСП.

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

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

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

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

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

public void AddUserAndNotify(IUserRepository repo, IEmailNotification notifier, MyUser user)
{
    repo.Add(user);
    repo.SaveChanges();
    notifier.SendUserCreatedNotification(user);
}

Но увеличивает ли это ценность, зависит от специфики вашего приложения.

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

Наиболее простой шаблон для управления транзакцией базы данных - это внешний usingблок:

using (DataContext context = new DataContext())
{
    _userRepository.Add(context, user);
    context.SaveChanges();
    notifier.SendUserCreatedNotification(user);
}

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

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

using (DataContext context = new DataContext())
{
    _userRepository.Add(context, user);
    _emailNotificationQueue.AddUserCreateNotification(user);
    _emailNotificationQueue.Commit();
    context.SaveChanges();
}

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

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

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

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

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

Либо вы инициируете событие первым, либо вы должны добавить к SaveChanges. Ваше решение является гибридом между ними. Это не относится к существующему нарушению, в то время как оно поощряет предотвращение его увеличения за пределы трех вещей. Реорганизация существующего кода для соответствия SRP может потребовать больше работы, чем это строго необходимо. Это зависит от вашего проекта, насколько далеко они хотят пройти SRP.

Код уже нарушил SRP - тот же класс отвечал за связь с контекстом данных и ведение журнала.

Вы просто повышаете его до 3-х обязанностей.

Один из способов избавиться от одной ответственности - абстрагировать _userRepository; сделай это командой-вещателем.

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

Теперь у большинства команд может быть только 1 слушатель (контекст данных). SaveChanges, до ваших изменений, имеет 2 - контекст данных, а затем регистратор.

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

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

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