40

I recently came by a seemingly trivial architectural problem. I had a simple repository in my code that was called like this (code is in C#):

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

SaveChanges was a simple wrapper that commits changes to database:

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

Then, after some time, I needed to implement new logic that would send email notifications every time a user was created in the system. Since there were many calls to _userRepository.Add() and SaveChanges around the system, I decided to update SaveChanges like this:

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

This way, external code could subscribe to UserCreatedEvent and handle the needed business logic that would send notifications.

But it was pointed out to me that my modification of SaveChanges violated the Single Responsibility principle, and that SaveChanges should just save and not fire any events.

Is this a valid point? It seems to me that the raising an event here is essentially the same thing as logging: just adding some side functionality to the function. And SRP does not prohibit you from using logging or firing events in your functions, it just says that such logic should be encapsulated in other classes, and it is OK for a repository to call these other classes.

  • 22
    Your retort is: "OK, so how would you write it so that it doesn't violate SRP but still allows a single point of modification?" – Robert Harvey Mar 26 at 18:09
  • 43
    My observation is that raising an event does not add an additional responsibility. In fact, quite the opposite: it delegates the responsibility somewhere else. – Robert Harvey Mar 26 at 18:10
  • I think your coworker is right, but your question is valid and useful, so upvoted! – Andres F. Mar 26 at 20:48
  • 16
    There's no such thing as a definitive definition of a Single Responsibility. The person pointing out that it violates SRP is correct using their personal definition and you are correct using your definition. I think your design is perfectly fine with the caveat that this event isn't a one-off whereby other similar functionality is done in different ways. Consistency is far, far, far more important to pay attention to than some vague guideline like SRP which carried to the extreme ends up with tons of very easy to understand classes that nobody knows how to make work in a system. – Dunk Mar 26 at 22:11
  • 2

10 Answers 10

14

Yes, it can be a valid requirement to have a repository which fires certain events on certain actions like Add or SaveChanges - and I am not going to question this (like some other answers) just because your specific example of adding users and sending emails may look a bit contrived. In the following, let us assume this requirement is perfectly justified in the context of your system.

So yes, encoding the event mechanics as well as the logging as well as the saving into one method violates the SRP. For lots of cases , it is probably an acceptable violation, especially when noone ever wants to distribute the maintenance responsibilities of "save changes" and "raise event" to different teams/maintainers. But lets assume one day someone wants to do exactly this, can it be resolved in a simple manner, maybe by putting the code of those concerns into different class libraries?

The solution to this is to let your original repository stay responsible for committing changes to database, nothing else, and make a proxy repository which has exactly the same public interface, reuses the original repository and adds the additional event mechanics to the methods.

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

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

You can call the proxy class a NotifyingRepository or ObservableRepository if you like, along the lines of @Peter's highly voted answer (which actually does not tell how to resolve the SRP violation, only saying the violation is ok).

The new and the old repository class should both derive from a common interface, like shown in the classic Proxy pattern description.

Then, in your original code, initialize _userRepository by an object of the new EventFiringUserRepo class. That way, you keep the original repository separated from the event mechanics. If required, you can have the event firing repository and the original repository side-by-side and let the callers decide if they use either the former or the latter.

To adress one concern mentioned in the comments: doesn't that lead to proxies on top of proxies on top of proxies, and so on? Actually, adding the event mechanics creates a foundation to add further requirements of the "send emails" type by simply subscribing to the events, so sticking to the SRP with those requirements as well, without any additional proxies. But the one thing which has to be added once here is the event mechanics itself.

If this kind of separation is really worth it in the context of your system is something you and your reviewer have to decide by yourself. I probably would not separate the logging from the original code, neither by using another proxy not by adding a logger to the listener event, though it would be possible.

  • 3
    In addition to this answer. There are alternatives to proxies, like AOP. – Laiv Mar 27 at 8:53
  • 1
    I think you miss the point, its not that raising an event breaks the SRP its that raising an event only for "New" users requires the repo to be responsible for knowing what constitutes a "New" user rather than a "Newly Added to Me" user – Ewan Mar 27 at 9:45
  • @Ewan: please read the question again. It was about a place in code which does certain actions which needs to be coupled with other actions outside of the responsivility of that object. And putting the action and the event raising in one place was questioned by a peer reviewer as breaking the SRP. The example of "saving new users" is only for the purpose of demonstration, call the example contrived if you like, but that is IMHO not the point of the question. – Doc Brown Mar 27 at 14:25
  • 2
    This is the best/correct answer IMO. Not only does it maintain SRP, but it also maintains the Open/Closed principle. And think of all of the automated tests that changes within the class could break. Modifying existing tests when new functionality is added is a big smell. – Keith Payne Mar 27 at 15:33
  • 1
    How does this solution work if there is a transaction in progress? When that is going on, SaveChanges() does not actually create the database record, and it could end up getting rolled back. Seems like you'd have to either override AcceptAllChanges or subscribe to the TransactionCompleted event. – John Wu Mar 27 at 23:17
29

Sending a notification that the persistent data store changed seems like a sensible thing to do when saving.

Of course you shouldn't treat Add as a special case - you'd have to fire events for Modify and Delete as well. It's the special treatment of the "Add" case that smells, forces the reader to explain why it smells, and ultimately leads some readers of the code to conclude it must violate SRP.

A "notifying" repository that can be queried, changed, and fires events on changes, is a perfectly normal object. You can expect to find multiple variations thereof in pretty much any decently sized project.


But is a "notifying" repository actually what you need? You mentioned C#: Many people would agree that using a System.Collections.ObjectModel.ObservableCollection<> instead of System.Collections.Generic.List<> when the latter is all you need is all kinds of bad and wrong, but few would immediately point to SRP.

What you are doing now is swapping your UserList _userRepository with an ObservableUserCollection _userRepository. If that's the best course of action or not depends on the application. But while it unquestionably makes the _userRepository considerably less lightweight, in my humble opinion it doesn't violate SRP.

  • The problem with using ObservableCollectionfor this case is, it triggers the equivalent event not at the call to SaveChanges, but at the call to Add, which would lead to a very different behaviour than the one shown in the example. See my answer how to keep the original repo lightweight, and still stick to the SRP by keeping the semantics intact. – Doc Brown Mar 27 at 17:03
  • @DocBrown I invoked the known classes ObservableCollection<> and List<> for comparison and context. I did not mean to recommend using the actual classes for either the internal implementation or the external interface. – Peter - Unban Robert Harvey Mar 27 at 18:27
  • Ok, but even if the OP would add events to "Modify" and "Delete" (which I think the OP left out to keep the question concise, for the sake of simplicity), I think a reviewer could easily come to the conclusion of an SRP violation. It is probably an acceptable one, but none which cannot be resolved if required. – Doc Brown Mar 27 at 20:35
16

Yes, it is a violation of the single responsibility principle and a valid point.

A better design would be to have a separate process retrieve 'new users' from the repository and send the emails. Keeping track of which users have been sent an email, failures, resends, etc., etc.

This way you can handle errors, crashes and the like as well as avoiding your repository grabbing every requirement which has the idea that events happen "when something is committed to the database".

The repository doesn't know that a user you add is a new user. Its responsibility is simply storing the user.

It's probably worth expanding on the comments below.

The repository doesn't know that a user you add is a new user - yes it does, it has a method called Add. Its semantics implies that all added users are new users. Combine all the arguments passed to Add before calling Save - and you get all new users

Incorrect. You are conflating "Added to the Repository" and "New".

"Added to the Repository" means just what it says. I can add and remove and re-add users to various repositories.

"New" is a state of a user defined by business rules.

Currently the business rule might be "New == just been added to the repository", but that doesn't mean it's not a separate responsibility to know about and apply that rule.

You have to be careful to avoid this kind of database-centric thinking. You will have edge case processes which add non-new users to the repository and when you send emails to them all the business will say "Of course those are not 'new' users! The actual rule is X"

This answer is IMHO quite missing the point: the repo is exactly the one central place in the code which knows when new users are added

Incorrect. For the reasons above, plus it's not a central location unless you actually include the email sending code in the class rather than just raising an event.

You will have applications which use the repository class, but don't have the code to send the email. When you add users in those applications the email will not be sent.

  • 11
    The repository doesn't know that a user you add is a new user - yes it does, it has a method called Add. Its semantics implies that all added users are new users. Combine all the arguments passed to Add before calling Save - and you get all new users. – Andre Borges Mar 26 at 23:29
  • I like this suggestion. However, pragmatism prevails over purity. Depending on the circumstances, adding an entirely new architectural layer to an existing application can be difficult to justify if all you need to do is literally send a single email when a user is added. – Alexander Mar 27 at 1:09
  • But the event is not saying user added. It says user created. If we consider naming things properly and we agree with the semantical differences between add and create, then the event in the snippet is either wrong named or missplaced. I don't think the reviewer had anything against notyfing repositories. Probably It was concerned about the kind of event and its side effects. – Laiv Mar 27 at 7:21
  • 7
    @Andre New to the repo, but not necessarily "new" in the business sense. its the conflation of these two ideas that's hiding the extra responsibility from first glance. I might import a tonne of old users to my new repository, or remove and re-add a user etc. There will be business rules about what a "new user" is beyond "has been added to the dB" – Ewan Mar 27 at 7:31
  • 1
    Moderator Note: Your answer isn't a journalistic interview. If you have edits, incorporate them naturally into your answer without creating the whole "breaking news" effect. We're not a discussion forum. – Robert Harvey Mar 27 at 18:28
7

Is this a valid point?

Yes it is, although it depends a lot on the structure of your code. I don't have the full context so I will try to talk in general.

It seems to me that the raising an event here is essentially the same thing as logging: just adding some side functionality to the function.

It absolutely isn't. Logging is not part of the business flow, it can be disabled, it shouldn't cause (business) side effects and should not influence the state and heath of your application in any way, even if you were for some reason not able to log anything anymore. Now compare that with the logic you added.

And SRP does not prohibit you from using logging or firing events in your functions, it just says that such logic should be encapsulated in other classes, and it is OK for a repository to call these other classes.

SRP works in tandem with ISP (S and I in SOLID). You end up with many classes and methods that do very specific things and nothing else. They are very focused, very easy to update or replace, and in general easy(er) to test. Of course in practice you'll also have a few bigger classes that deal with the orchestration: they will have a number of dependencies, and they will focus not on atomised actions, but on business actions, which may require multiple steps. As long as the business context is clear, they can too be called single responsibility, but as you correctly said, as the code grows, you may want to abstract some of it into new classes / interfaces.

Now back to your particular example. If you absolutely must send a notification whenever a user is created and maybe even perform other more specialised actions, then you could create a separate service that encapsulates this requirement, something like UserCreationService, which exposes one method, Add(user), which handles both the storage (the call to your repository) and the notification as a single business action. Or do it in your original snippet, after _userRepository.SaveChanges();

  • 2
    Logging is not part of the business flow - how is this relevant in the context of SRP? If the purpose of my event would be to send new user data to Google Analytics - then disabling it would have the same business effect as disabling logging: not critical, but pretty upsetting. What is the rule of a thumb for adding/not adding new logic to a function? "Will disabling it cause major business side effects?" – Andre Borges Mar 26 at 23:24
  • 2
    If the purpose of my event would be to send new user data to Google Analytics - then disabling it would have the same business effect as disabling logging: not critical, but pretty upsetting . What if you are firing premature events causing fake "news". What if the analytics are taking into account "users" that were not finally created due to errors with the DB transaction? What if the company is making decisions upon false premises, backed by imprecise data? You are too focused on the technical side of the issue. "Sometimes you can't see the wood for the trees'" – Laiv Mar 28 at 6:48
  • @Laiv, you are making a valid point, but this is not the point of my question, or this answer. The question is whether this is a valid solution in the context of SRP, so let's assume there are no DB transaction errors. – Andre Borges Mar 28 at 9:27
  • You are basically asking me to tell you what you want to hear. I just give you scope. A wider scope for you to decide whether SRP matters or not because SRP is useless without the proper context. IMO the way you are approaching the concern is incorrect because you are only focusing on the technical solution. You should give enough relevance to the whole context. And yes, DB might fail. There's a chance for it to happen and you should not omit that, because as you know, things happen and these things could change your mind regarding doubts about SRP or other good practices. – Laiv Mar 28 at 11:10
  • 1
    That said, remember that principles are not rules written in stone. They are permeable (adaptive). As you can see, they are open to interpretation. Your reviewer has an interpretation and you have another. Try to see what you see, resolve his/her doubts and concerns, or let him/her solve yours. You will not find the "right" answer here. The correct answer is up to you and your reviewer to find, asking first to the requirements (functional and non-functional) of the project. – Laiv Mar 28 at 11:17
4

SRP is, theoretically, about people, as Uncle Bob explains in his article The Single Responsibility Principle. Thanks Robert Harvey for providing it in your comment.

The correct question is:

Which "stakeholder" added the "send emails" requirement?

If that stakeholder is also in charge of data persistence (unlikely but possible) then this does not violate SRP. Otherwise, it does.

  • 2
    Interesting - I never heard of this interpretation of the SRP. Do you have any pointers to more information / literature about this interpretation? – sleske Mar 27 at 14:56
  • 2
    @sleske: From Uncle Bob himself: "And this gets to the crux of the Single Responsibility Principle. This principle is about people. When you write a software module, you want to make sure that when changes are requested, those changes can only originate from a single person, or rather, a single tightly coupled group of people representing a single narrowly defined business function." – Robert Harvey Mar 27 at 18:12
  • Thanks Robert. IMO, The name "Single Responsibility Principle" is terrible, as it sounds simple, but too few people follow up on what is the intended meaning of "responsibility". Kind of like how OOP has mutated from many of it's original concepts, and is now a fairly meaningless term. – user949300 Mar 27 at 18:28
  • 1
    Yep. That's what happened to the term REST. Even Roy Fielding says people are using it wrong. – Robert Harvey Mar 27 at 18:32
  • Though the cite is related, I think this answer misses that the "send emails" requirement is none of the direct requirements of which the SRP violation question is about. However, by saying "Which "stakeholder" added the "raise event" requirement", this answer would become more related to the actual question. I modified my own answer a little bit to make this more clear. – Doc Brown Mar 27 at 20:46
2

While technically there's nothing wrong with repositories notifying events, I would suggest looking at it from a functional point of view where its convenience rises some concerns.

Creating a user, deciding what's a new user is and its persistence are 3 different things.

Premise of mine

Consider the previous premise before deciding whether the repository is the proper place to notify business events (regardless of the SRP). Note that I said business event because to me UserCreated has a different connotation than UserStored or UserAdded 1. I would also consider each of those events to be addressed to different audiences.

On one side, creating users is a business-specific rule that might or might not involve persistence. It might involve more business operations, involving more database/network operations. Operations the persistence layer is unaware of. The persistence layer doesn't have enough context to decide whether the use case ended successfully or not.

On the flip side, it's not necessarily true that _dataContext.SaveChanges(); has persisted the user successfully. It will depend on the database's transaction span. For instance, it could be true for databases like MongoDB, which transactions are atomic, but it could not, for traditional RDBMS implementing ACID transactions where there could be more transactions involved and yet to be committed.

Is this a valid point?

It could be. However, I would dare to say that it's not only a matter of SRP (technically speaking), it's also a matter of convenience (functionally speaking).

  • Is it convenient to fire business events from components unaware of the business operations in progress?
  • Do they represent the right place as much as the right moment to do it?
  • Should I allow these components to orchestrate my business logic through notifications like this?
  • Could I invalidate the side effects caused by premature events? 2

It seems to me that the raising an event here is essentially the same thing as logging

Absolutely not. Logging is meant to have no side effects, however, as you suggested the event UserCreated is likely to cause other business operations to happen. Like notifications. 3

it just says that such logic should be encapsulated in other classes, and it is OK for a repository to call these other classes

Not necessarily true. SRP is not a class-specific concern only. It operates at different levels of abstractions, like layers, libraries and systems! It's about cohesion, about keeping together what changes for the same reasons by the hand of the same stakeholders. If the user creation (use case) changes it's likely the moment and the reasons for the event to happen also changes.


1: Naming things adequately also matters.

2: Say we sent UserCreated after _dataContext.SaveChanges();, but the whole database transaction failed later due to connection issues or constraints violations. Be careful with premature broadcasting of events, because its side effects can be hard to undo (if that is even possible).

3: Notification processes not treated adequately might cause you to fire notifications that can not be undone/sup>

  • 1
    +1 Very good point about the transaction span. It can be premature to assert the user has been created, because rollbacks can happen; and unlike with a log, it's likely some other part of the app does something with the event. – Andres F. Mar 26 at 20:47
  • 2
    Exactly. Events denote certainity. Something happened but it's over. – Laiv Mar 26 at 20:50
  • 1
    @Laiv: Except when they don't. Microsoft has all sorts of events prefixed with Before or Preview that make no guarantees at all about certainty. – Robert Harvey Mar 26 at 21:17
  • 1
    @jpmc26: Without an alternative, your suggestion is not helpful. – Robert Harvey Mar 27 at 18:10
  • 1
    @jpmc26: So your answer is "change to a completely different development ecosystem with an entirely different set of tools and performance characteristics." Call me contrary, but I would imagine this is infeasible for the vast majority of development efforts. – Robert Harvey Mar 27 at 18:14
1

No this does not violate the SRP.

Many seem to think the Single Responsibility Principle means a function should only do "one thing", and then get caught up in discussion about what constitute "one thing".

But that is not what the principle means. It is about business-level concerns. A class should not implement multiple concerns or requirements which may change independently at the business level. Lets say a class both stores the user and sends a hardcoded welcome message via email. Multiple independent concerns could cause the requirements of such a class to change. The designer could require the html/stylesheet of the mail to change. The communications expert could require the wording of the mail to change. And the UX expert could decide the mail should actually be sent at a different point in the onboarding flow. So the class is subject to multiple requirement changes from independent sources. This violates the SRP.

But firing an event does not violate the SRP then, since the event only depends on saving the user and not on any other concern. Events are actually a really nice way to uphold the SRP, since you can have en email triggered by the save without the repository being affected by - or even knowing about - the mail.

1

Don't worry about single responsibility principle. It's not going to help you make a good decision here because you can subjectively choose a particular concept as a "responsibility." You could say the class' responsibility is managing data persistence to the database, or you could say its responsibility is to perform all the work related to creating a user. These are just different levels of the application's behavior, and they're both valid conceptual expressions of a "single responsibility." So this principle is unhelpful for solving your problem.

The most useful principle to apply in this case is the principle of least surprise. So let's ask the question: is it surprising that a repository with the primary role of persisting data to a database also sends e-mails?

Yes, it very much is surprising. These are two completely separate external systems, and the name SaveChanges does not imply also sending notifications. The fact you delegate this out to an event makes the behavior even more surprising, since someone reading the code can no longer easily see what additional behaviors are invoked. Indirection harms readability. Sometimes, the benefits are worth the readability costs, but not when you're automatically invoking an additional external system that has effects observable to end users. (Logging can be excluded here since its effect is essentially record keeping for debugging purposes. End users do not consume the log, so there is no harm in always logging.) Even worse, this reduces flexibility in the timing of sending the e-mail, making it impossible to interleave other operations between the save and the notification.

If your code typically needs to send a notification when a user is successfully created, you could create a method that does so:

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

But whether this adds value depends on your application's specifics.


I'd actually discourage the existence of the SaveChanges method at all. This method will presumably commit a database transaction, but other repositories might have modified the database in the same transaction. The fact it commits all of them is again surprising, since SaveChanges is specifically tied to this instance of the user repository.

The most straightforward pattern for managing a database transaction is an outer using block:

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

This gives the programmer explicit control over when changes for all repositories are saved, forces the code to explicitly document the sequence of events that must occur before a commit, ensures a rollback is issued on error (assuming that DataContext.Dispose issues a rollback), and avoids hidden connections between stateful classes.

I'd also prefer not to send the e-mail directly in the request. It would be more robust to record the need for a notification in a queue. This would allow for better failure handling. In particular, if an error occurs sending the e-mail, it can be tried again later without interrupting saving the user, and it avoids the case where the user is created but an error is returned by the site.

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

It's better to commit the notification queue first since the queue's consumer can verify that the user exists before sending the e-mail, in the event that the context.SaveChanges() call fails. (Otherwise, you'll need a full-blown two-phase commit strategy to avoid heisenbugs.)


The bottom line is to be practical. Actually think through the consequences (both in terms of risk and benefit) of writing code a particular way. I find that "single responsibility principle" doesn't very often help me do that, while "principle of least surprise" often helps me get into another developer's head (so to speak) and think about what might happen.

  • 4
    is it surprising that a repository with the primary role of persisting data to a database also sends e-mails - I think you missed the point of my question. My repository is not sending emails. It just raises an event, and how this event is handled - is the responsibility of external code. – Andre Borges Mar 27 at 17:55
  • 4
    You're basically making the argument "don't use events." – Robert Harvey Mar 27 at 18:03
  • 3
    [shrug] Events are central to most UI frameworks. Eliminate events, and those frameworks don't work at all. – Robert Harvey Mar 27 at 18:06
  • 2
    @jpmc26: It's called ASP.NET Webforms. It sucks. – Robert Harvey Mar 27 at 18:21
  • 2
    My repository is not sending emails. It just raises an event cause-effect. The repository is triggering the notification process. – Laiv Mar 28 at 7:14
0

Currently SaveChanges does two things: it saves the changes and logs that it does so. Now you want to add another thing to it: send email notifications.

You had the clever idea to add an event to it, but this was criticised for violating the Single Responsibility Principle (SRP), without noticing that it had already been violated.

To get a pure SRP solution, first trigger the event, then call all the hooks for that event, of which there are now three: saving, logging, and finally sending emails.

Either you trigger the event first, or you have to add to SaveChanges. Your solution is a hybrid between the two. It doesn't address the existing violation while it does encourage preventing it from increasing beyond three things. Refactoring the existing code to comply with SRP might require more work than is strictly necessary. It's up to your project how far they want to take SRP.

-1

The code already violated the SRP -- the same class was responsible for communicating with the data context and logging.

You just upgrade it to having 3 responsibilities.

One way to strip things back to 1 responsibility would be to abstract the _userRepository; make it a command-broadcaster.

It has a set of commands, plus a set of listeners. It gets commands, and broadcasts them to its listeners. Possibly those listeners are ordered, and maybe they can even say the command failed (which in turn is broadcast to listeners who had already been notified).

Now, most of the commands may have only 1 listener (the data context). SaveChanges, prior to your changes, has 2 -- the data context, and then the logger.

Your change then adds another listener to save changes, which is to raise new user created events in the event service.

There are a few benefits to this. You can now remove, upgrade, or replicate the logging code without the rest of your code caring. You can add more triggers at the save changes for more things that need it.

All of this gets decided when the _userRepository is created and wired up (or, maybe, those extra features get added/removed on the fly; being able to add/enhance logging while the application runs could be of use).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.