Btukfyl

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.

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. It's responsibility is simply storing the user.

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

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.

Be careful with premature event launching, because its side effects are hard (if possible) to undo.

That said, consider the next premise. Creating users is one thing, it's persistence a different one.

Creating users is a business-specific rule. A business concern. It might or might not involve persistence. It might involve more business operations, involving at the same time more database/network operations. Operations the persistence layer knows nothing about (and should not).

It's not even true that _dataContext.SaveChanges(); has persisted the user successfully. It will depend on the db' transaction span. It could be true for databases like Mongodb, which transactions are atomic, but It could not for traditional RDBS implementing ACID transactions.

There's a reason why transaction management happens at the business or service level. These are levels closer to the semantics of the business. They usually describe what user creation means, what to do when everything goes ok and what to do when not.

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

Note even close. Logging has no side effects. At least not the ones application events could have.

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 true. SRP is not a class-specific concern. It also operates at higher-levels of abstractions, like layers, components, systems! It's about cohesion, 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.

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.