2

Let's say, I have created a business logic layer service (or a handler in Command/Query pattern) with a method DoSomething which processes a bunch of entities and stores them in database.

Later on, another developer sees - "oh, this DoSomething method does almost everything I need, I can call it from my DoThisSomethingAndMore method." Seems the right thing to do to avoid copy-pasting the code from DoSomething into DoThisSomethingAndMore. Unit tests pass, everything is OK.

Then a new business requirement comes in to modify DoSomething operation. So, I change the code of DoSomething, and thus DoThisSomethingAndMore becomes broken.

If we have good tests, we can detect it early on. But how to fix the issue? Make DoThisSomethingAndMore not to call DoSomething and copy-paste the code instead? Refactor DoSomething into pieces that can be reused from both DoSomething and DoThisSomethingAndMore? This might be a noticeable amount of work, involving all the developers who are calling DoSomething from their code and cascade to even more code which is calling other methods. Also, this might lead to a bunch of public methods (DoPart1OfSomething, DoPart2OfSomething ...) on the business layer service class, when these methods should really be internal because they don't actually perform a single atomic business operation but just a part of it.

Is this a common programming issue and should it always be solved by refactoring or does it mean that we should go for full business service and operation decoupling, accepting that it will lead to some code duplication?

Inner voice says - make each developer responsible only for his own code in business methods he created and do not call others' methods when you know that they, most probably, will change their inner implementation. This seems reasonable considering that the project is being developed by a startup company with some not so experienced developers who would benefit from clear "rule of thumb" since the very beginning instead of investigating each method call separately, and also considering that business requirements are changing often during first phases of development.

And then there's another inner voice "hey, don't duplicate the code; code reuse is one of the best practices".

So, while I would like to go with the single responsibility with code duplication approach, I cannot find a reasonable excuse or solid foundation for it. Is there any? It would be great to have a reference to some reputable source (e.g. "The Gang of Four") saying in which cases code duplication is generally acceptable.

  • 4
    This is awfully abstract. In many real-life cases, chances are that a change request for DoSomething would equally apply to DoSomethingAndMore (even if the requester doesn't remember to make that request as well), so that the second routine doesn't, in fact, break. At other times, the change obviously doesn't apply further than indicated. But which it is in your case, i.e. whether the reuse was a good thing to do or not, would require more information about what it actually is that these methods do. – Kilian Foth Jul 6 '16 at 11:07
  • @Kilian Foth Yes, that makes sense, but that leads to "it depends" and individual case investigation, which complicates things. – JustAMartin Jul 6 '16 at 12:08
2

If someone is going to change a method which is reused somewhere in your codebase, he should be responsible for making an impact analysis before. So a dev who starts to implement a change in DoSomething should check beforehand from where this method is called. So lets assume you already found out DoThisSomethingAndMore won't work any more correctly when doing the change naively.

To fix the issue, there are several possibilities. The very wrong way is to duplicate the implementation code of DoSomething. Refactoring DoSomething into pieces which can be reused from both methods is a viable option. If that is "a noticeable amount of work", this is probably a sign the method is already too big and too complicated, and the earlier you start to refactor into smaller building blocks, the better.

If there are lots of places where DoSomething was used throughout the code, and the old behaviour should be kept in most of the places where the reusage took place, the other option is to make sure DoSomething itself does not change its behaviour, and implement the new requirement in new a method DoSomethingNew. Refactoring DoSomething into pieces which can be reused from DoSomething and DoSomethingNew should be the way to go, but now it is a true refactoring, because it does not change the behaviour of DoSomething, and none of the methods using this method needs to be changed.

0

IMO I would duplicate the code, make the necessary changes and then refactor whetever duplication remains. Sometimes, you have to make a mess before you can start cleaning up.

But I would agree with Kilian Foth's comment. There is something much deeper here. The problem is that if this case happens, then DoSomething was not a good method int the first place. It didn't represent actual single responsibility that it was supposed to do. For example, if it had 2 responsibilities, then change to one of those responsibilities might break code that also uses the other responsibility but doesn't want change to the first one. So it should actually be two different methods. And my suggestion above would result in this becoming obvious.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.