Hcfyk

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.

--Edit

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

SRP is, theoretically, about people. 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.

Yes, it can be a valid requirement to have a repo 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 is requirement is perfectly justfied 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 repo 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 repo 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 repo 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 repo separated from the event mechanics. If required, you can have the event firing repo and the original repo 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 priciple.

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}

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.