Applicability of Single Responsibility PrincipleDo you leverage the benefits of the open-closed principle?Single Responsibility Principle ImplementationSomething confusing about Single Responsibility PrincipleSingle Responsibility Principle: Responsibility unknownIdentifying Domain Services & Application Services when doing DDDApplication of Single Responsibility Principle on a ButtonWhen using the Single Responsibility Principle, what constitutes a “responsibility?”Single Responsibility Principle Violation?Does Template pattern violate Single Responsibility principle?Single responsibility principle - importer
Will it be accepted, if there is no ''Main Character" stereotype?
How do I keep an essay about "feeling flat" from feeling flat?
Is it correct to write "is not focus on"?
How does residential electricity work?
What is the oldest known work of fiction?
Is there a good way to store credentials outside of a password manager?
Is there a measurement for the vocal speed of a song?
apt-get update is failing in debian
Is exact Kanji stroke length important?
If a character can use a +X magic weapon as a spellcasting focus, does it add the bonus to spell attacks or spell save DCs?
Should my PhD thesis be submitted under my legal name?
How can a jailer prevent the Forge Cleric's Artisan's Blessing from being used?
I'm in charge of equipment buying but no one's ever happy with what I choose. How to fix this?
Lay out the Carpet
Can I Retrieve Email Addresses from BCC?
Is it okay / does it make sense for another player to join a running game of Munchkin?
Displaying the order of the columns of a table
Go Pregnant or Go Home
Greatest common substring
How does a character multiclassing into warlock get a focus?
Tiptoe or tiphoof? Adjusting words to better fit fantasy races
How to be diplomatic in refusing to write code that breaches the privacy of our users
Cynical novel that describes an America ruled by the media, arms manufacturers, and ethnic figureheads
What would happen if the UK refused to take part in EU Parliamentary elections?
Applicability of Single Responsibility Principle
Do you leverage the benefits of the open-closed principle?Single Responsibility Principle ImplementationSomething confusing about Single Responsibility PrincipleSingle Responsibility Principle: Responsibility unknownIdentifying Domain Services & Application Services when doing DDDApplication of Single Responsibility Principle on a ButtonWhen using the Single Responsibility Principle, what constitutes a “responsibility?”Single Responsibility Principle Violation?Does Template pattern violate Single Responsibility principle?Single responsibility principle - importer
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.
architecture single-responsibility
add a comment |
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.
architecture single-responsibility
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♦
4 hours ago
2
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♦
4 hours ago
I think your coworker is right, but your question is valid and useful, so upvoted!
– Andres F.
1 hour ago
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
19 mins ago
add a comment |
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.
architecture single-responsibility
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.
architecture single-responsibility
architecture single-responsibility
asked 4 hours ago
Andre BorgesAndre Borges
6291811
6291811
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♦
4 hours ago
2
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♦
4 hours ago
I think your coworker is right, but your question is valid and useful, so upvoted!
– Andres F.
1 hour ago
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
19 mins ago
add a comment |
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♦
4 hours ago
2
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♦
4 hours ago
I think your coworker is right, but your question is valid and useful, so upvoted!
– Andres F.
1 hour ago
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
19 mins ago
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♦
4 hours ago
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♦
4 hours ago
2
2
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♦
4 hours ago
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♦
4 hours ago
I think your coworker is right, but your question is valid and useful, so upvoted!
– Andres F.
1 hour ago
I think your coworker is right, but your question is valid and useful, so upvoted!
– Andres F.
1 hour ago
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
19 mins ago
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
19 mins ago
add a comment |
4 Answers
4
active
oldest
votes
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.
add a comment |
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.
+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.
1 hour ago
1
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
@Laiv: Except when they don't. Microsoft has all sorts of events prefixed withBefore
orPreview
that make no guarantees at all about certainty.
– Robert Harvey♦
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
add a comment |
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();
add a comment |
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.
add a comment |
Your Answer
StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "131"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);
else
createEditor();
);
function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);
);
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fsoftwareengineering.stackexchange.com%2fquestions%2f389237%2fapplicability-of-single-responsibility-principle%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
4 Answers
4
active
oldest
votes
4 Answers
4
active
oldest
votes
active
oldest
votes
active
oldest
votes
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.
add a comment |
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.
add a comment |
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.
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.
edited 4 hours ago
answered 4 hours ago
EwanEwan
42.2k33593
42.2k33593
add a comment |
add a comment |
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.
+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.
1 hour ago
1
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
@Laiv: Except when they don't. Microsoft has all sorts of events prefixed withBefore
orPreview
that make no guarantees at all about certainty.
– Robert Harvey♦
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
add a comment |
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.
+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.
1 hour ago
1
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
@Laiv: Except when they don't. Microsoft has all sorts of events prefixed withBefore
orPreview
that make no guarantees at all about certainty.
– Robert Harvey♦
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
add a comment |
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.
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.
edited 1 hour ago
answered 2 hours ago
LaivLaiv
6,89311241
6,89311241
+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.
1 hour ago
1
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
@Laiv: Except when they don't. Microsoft has all sorts of events prefixed withBefore
orPreview
that make no guarantees at all about certainty.
– Robert Harvey♦
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
add a comment |
+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.
1 hour ago
1
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
@Laiv: Except when they don't. Microsoft has all sorts of events prefixed withBefore
orPreview
that make no guarantees at all about certainty.
– Robert Harvey♦
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
+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.
1 hour ago
+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.
1 hour ago
1
1
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
Exactly. Events denote certainity. Something happened but it's over.
– Laiv
1 hour ago
@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♦
1 hour ago
@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♦
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
Hmm. I have never seen this sort of events. Would you mind sharing some references?
– Laiv
1 hour ago
add a comment |
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();
add a comment |
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();
add a comment |
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();
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();
answered 1 hour ago
asyncasync
51459
51459
add a comment |
add a comment |
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.
add a comment |
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.
add a comment |
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.
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.
edited 50 mins ago
answered 56 mins ago
user949300user949300
5,84511528
5,84511528
add a comment |
add a comment |
Thanks for contributing an answer to Software Engineering Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fsoftwareengineering.stackexchange.com%2fquestions%2f389237%2fapplicability-of-single-responsibility-principle%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
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♦
4 hours ago
2
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♦
4 hours ago
I think your coworker is right, but your question is valid and useful, so upvoted!
– Andres F.
1 hour ago
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
19 mins ago