3 ms·

Sorry about misunderstanding. I think that's where SRP and naming comes in like mentioned elsewhere in the thread. If methods really do one simple thing and ar

Sorry about misunderstanding. I think that's where SRP and naming comes in like mentioned elsewhere in the thread. If methods really do one simple thing and are properly named, it's easy to tell if they should be re-used or not. A lot of the time, side-effects are not mentioned in method names or params (or the params are optional, that's where the issue comes in). E.g. SaveCandidate(Candidate candidate); In reality, SaveCandidate does two things, saves the candidate and updates their "ProfileCompletionStatus". If someone sees SaveCandidate and not SaveCandidateAndUpdateCompletionStatus or SaveCandidate(candidate, SaveSideEffects.DO_UPDATE_COMPLETION_STATUS) they will re-use SaveCandidate in places here completion status shouldn't be updated (contrived example - data migrations). The problem, of course, is that these overly-clear, overly-detailed methods are a pain to read. The common advice is "it's separate", but then we are repeating SaveCandidate(candidate); UpdateProfileCompletionStatus(candidate); everywhere, despite them being generally one process. I guess this is all pretty pedantic. When you spend a ton of time on this, you are missing out on getting stories done, but if you don't, the code eventually becomes umanageable.

lostcolony 6y ago
Right, but then someone sees that, and goes "Ah-hah. This is saving Candidates. Over here I'm also saving Foos. I'll refactor this so I have a generic 'Save' method/function, and then both Candidate and Foo can call it. DRY! And that works, because initially saving is just taking a serializable object and dumping it to JSON. But then things get moved to going to a DB or something, and now it has different expectations, and, oh, crap, we have to do some special logic in the transaction because it turns out saving a foo requires us holding a lock on the bar table as well, but not for candidates, and etc etc etc, and saving was -really just two lines of code in the first place-, and the correct solution is isolating it entirely (with maybe a Saver interface to say 'yeah, this thing knows how to save itself'. Though that likely breaks SRP since now Foos know how to be Foos, and how to save Foos, which involves knowledge of the DB, which seems suboptimal, but which is still cleaner than what you had before) Etc. My only point is that if the area you're tempted to repeat is not provably the same, contextually, keep 'em separate until it is (even if it's like...they implement the same interface). The pain of getting that wrong is almost always less than combining them, building more assumptions on top of that, and then finding out they should have been separate.

lostcolony 6y ago

Right, but then someone sees that, and goes "Ah-hah. This is saving Candidates. Over here I'm also saving Foos. I'll refactor this so I have a generic 'Save' method/function, and then both Candidate and Foo can call it. DRY! And that works, because initially saving is just taking a serializable object and dumping it to JSON. But then things get moved to going to a DB or something, and now it has different expectations, and, oh, crap, we have to do some special logic in the transaction because it turns out saving a foo requires us holding a lock on the bar table as well, but not for candidates, and etc etc etc, and saving was -really just two lines of code in the first place-, and the correct solution is isolating it entirely (with maybe a Saver interface to say 'yeah, this thing knows how to save itself'. Though that likely breaks SRP since now Foos know how to be Foos, and how to save Foos, which involves knowledge of the DB, which seems suboptimal, but which is still cleaner than what you had before) Etc. My only point is that if the area you're tempted to repeat is not provably the same, contextually, keep 'em separate until it is (even if it's like...they implement the same interface). The pain of getting that wrong is almost always less than combining them, building more assumptions on top of that, and then finding out they should have been separate.