Hfrhyu

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.

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.

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();

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), so let us assume this requirement is perfectly justified in the context of your system. And yes, encoding the event mechanics as well as the logging as well as the saving into one method violates the SRP (to a certain degree).

The solution to this is to let your original repository stay to be 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:
void SaveChanges()

 _basicRepo.SaveChanges();
 FireEventsForNewlyAddedUsers();


void FireEventsForNewlyAddedUsers()

 foreach (var newUser in _basicRepo.DataContext.GetAddedUsers())
 
 _eventService.RaiseEvent(new UserCreatedEvent(newUser))
 

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.

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 using the same approach, though it would be possible in principle.

SRP is, theoretically, about people. Uncle Bob Link added Here (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.

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 users is one thing, its persistence a different one.

^{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 ¹. 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?

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 cast business events from components that are not totally aware of the business operations in progress? Do they represent the right place as much as the right moment? Should the persistence orchestrate my business through notifications like this? Can we invalidate premature events?* ²

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, I suspect your event UserCreated is likely to cause other business operations to happen. Like notifications. ³

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. 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: Bear in mind that a sent email cannot be undon}

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.

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.

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).