55

The usual instinct is to remove any code duplication that you see in the code. However, I found myself in a situation where the duplication is illusory.

To describe the situation in more details: I am developing a web application, and most views are basically the same - they display a list of items which the user can scroll and choose from, a second list that contains selected items, and a "Save" button to save the new list.

It seemed to me that the problem is easy. However, each and every view has its own quirks - sometimes you need to recalculate something, sometimes you must store some additional data etc. These, I solved by inserting callback hooks in the main logic code.

There are so many minute differences between the views that it is becoming less and less maintainable, because I need to provide callbacks for basically all functionality, and the main logic starts to look like a huge sequence of callback invocations. In the end I am not saving any time or code, because every view has its own code that is executed - all in callbacks.

The problems are:

  • the differences are so minute that the code looks almost exactly alike in all views,
  • there are so many differences that when you look at the details, to code is not a bit alike

How should I handle this situation?
Is having core logic composed entirely of callback calls a good solution?
Or should I rather duplicate the code and drop the complexity of callback-based code?

  • 38
    I usually find it helpful to let the duplication go initially. Once I have a few examples, its much easier to see what's common and what's not and come up with a way to share the common parts. – Winston Ewert Oct 16 '15 at 16:18
  • 7
    Very good question - one thing to consider is not just the physical duplication of the code but the semantic duplication. If a change in one piece of the code necessarily means the same change would be duplicated across the others, then that part is probably a candidate for refactoring or abstraction. Sometimes you can normalise to the point where you actually trap yourself, so I would also consider the practical implications for treating the duplication as semantically distinct - they may be outweighed by the consequences of trying to deduplicate. – Ant P Oct 16 '15 at 16:42
  • Remember, you can only reuse code that does the same thing. If your app does different things on different screens, it will require different callbacks. No ifs, ands, or buts about it. – corsiKa Oct 16 '15 at 18:54
  • 13
    My personal rule of thumb on this is: If I make a change to the code in one place, is it a bug if I don't make the exact same change everywhere else? If so, it's the bad kind of duplication. If I'm not sure, go with whichever's more readable for now. In your example the differences in behavior are intentional, and not considered bugs, thus some duplication is fine. – Ixrec Oct 16 '15 at 19:04
  • You might be interested to read about Aspect Oriented Programming. – Ben Jackson Oct 17 '15 at 2:12
53

Ultimately you have to make a judgment call about whether to combine similar code to eliminate duplication.

There seems to be an unfortunate tendency to take principles like "Don't repeat yourself" as rules that must be followed by rote at all times. In fact, these are not universal rules but guidelines that should help you think about and develop good design.

As everything in life, you must consider the benefits versus the costs. How much duplicated code will be removed? How many times is the code repeated? How much effort will it be to write a more generic design? How much are you likely to develop the code in the future? And so on.

Without knowing your specific code, this is unclear. Perhaps there is a more elegant way to remove duplication (such as that suggested by LindaJeanne). Or, perhaps there simply isn't enough true repetition to warrant abstraction.

Insufficient attention to design is a pitfall, but also beware over-design.

  • Your comment about "unfortunate tendencies" and blindly following guidelines is on spot, I think. – Mael Oct 16 '15 at 12:11
  • 1
    @Mael Are you saying that if you wouldn't maintain this code in the future, you wouldn't have good reasons to get the right design ? (no offense, just want to know what you think about that) – Spotted Oct 16 '15 at 12:32
  • 2
    @Mael Of course we could consider it just an unfortunate turn of phrase ! :D However, I think we should be as strict with ourselves as we are for the others when writing code (I consider myself as an other when I read my own code 2 weeks after writing it). – Spotted Oct 16 '15 at 13:07
  • 2
    @user61852 then you would very much dislike The Codeless Code. – RubberDuck Oct 16 '15 at 16:02
  • 1
    @user61852, haha--but what if it does all depend (on information not given in the question)? Few things are less helpful than excess certainty. – user82096 Oct 16 '15 at 19:28
42

Remember that DRY is about knowledge. It doesn't matter whether two pieces of code look similar, identical, or totally different, what matters is if the same piece of knowledge about your system can be found in both of them.

A piece of knowledge might be a fact ("the maximum allowed deviation from the intended value is 0.1%") or it might be some aspect of your process ("this queue never contains more than three items"). It's essentially any single piece of information encoded in your source code.

So when you're deciding whether something is duplication that should be removed, ask whether it's duplication of knowledge. If not, it's probably incidental duplication, and extracting it to some common place will cause problems when you later want to create a similar component where that apparently duplicated part is different.

  • 12
    This! The focus of DRY is avoiding duplicate changes. – Matthieu M. Oct 16 '15 at 17:52
  • This is quite helpful. – user82096 Oct 16 '15 at 19:26
  • 1
    I thought the focus of DRY was to ensure that there are no two bits of code that should behave identical but don't. The problem isn't the doubled work because code changes have to be applied twice, the real problem is when a code change needs to be applied twice but isn't. – gnasher729 Oct 16 '15 at 21:14
  • 3
    @gnasher729 Yep, that is the point. If two pieces of code have duplication of knowledge, then you'd expect that when one needs to change, then the other will need to change too, leading to the problem you describe. If they have incidental duplication, then when one needs to change, the other may well need to stay the same. In that case if you've extracted a common method (or whatever), you now have a different problem to deal with – Ben Aaronson Oct 16 '15 at 21:20
  • 1
    Also essential duplication and accidental duplication, see An Accidental Doppelgänger in Ruby and I DRY-ed Up My Code and Now It’s Hard to Work With. What Happened?. Accidental duplicates also occur on both sides of a context boundary. Summary: only merge duplicates if it makes sense for their clients for these dependencies to be modified simultaneously. – Eric Oct 18 '15 at 12:28
27

Have you considered using a Strategy pattern? You would have one View class that contains the common code & routines called by several views. Children of the View class would contain the code specific to those instances. They would all use the common interface you created for the View, and thus the differences would be encapsulated & coherent.

  • 5
    No, I have not considered it. Thank you for the suggestion. From a quick read about the Strategy pattern it seems like something I am looking for. I'll definitely investigate further. – Mael Oct 16 '15 at 11:56
  • 3
    there is template method pattern. You can also consider that – Shakil Oct 21 '15 at 19:36
5

What's the potential for change? For example, our application has 8 different business areas with a potential of 4 or more user types for each area. Views are customized based on the user type and the area.

Initially, this was done using the same view with a few checks here and there to determine if different things should show. Over time, some of the business areas have decided to do drastically different things. In the end, we basically migrated to one view (partial views, in the case of ASP.NET MVC) per piece of functionality per business area. Not all business areas have the same functionality, but if one wants functionality that another has, that area gets its own view. It's a lot less cumbersome for comprehension of the code, as well as for testability. E.g., making a change for one area won't cause an unwanted change for another area.

As @dan1111 mentioned, it can come down to a judgment call. In time, you may find whether it works or not.

2

One issue might be that you are providing an interface (theoretical interface, not language feature) to only a single level of the functionality:

A(a,b,c) //a,b,c are your callbacks or other dependencies

Instead of multiple levels depending on how much control is required:

//high level
A(a,b,c)
//lower
A myA(a,b)
B(myA,c)
//even lower
A myA(a)
B myB(myA,b)
C myC(myB,c)
//all the way down to you just having to write the code yourself

As far as I understood, you only expose the high level interface (A), hiding the implementation details (the other things there).

Hiding implementation details has advantages, and you just found a disadvantage - control is limited, unless you explicitly add features for every single thing that would have been possible when directly using the low level interface(s).

So, you have two options. Either you only use the low level interface, use the low level interface because the high level interface was too much work to maintain, or expose both high and low level interfaces. The only sensible option is to offer both high and low level interfaces (and everything inbetween), assuming you want to avoid redundant code.

Then when writing another one of your things, you look at all the available functionality you have written that far (countless possibilities, up to you to decide which ones might be reused) and piece them together.

Use a single object where you need little control.

Use the lowest level functionality when some weirdness needs to happen.

Its also not very black-and-white. Maybe your big high level class CAN reasonably cover all the possible use cases. Maybe the use cases are so varying that nothing but the lowest level primitive functionality suffices. Up to you to find the balance.

1

There are already other useful answers. I will add mine.

Duplication is bad because

  1. it clutters the code
  2. it clutters our comphrension of the code but most importantly
  3. because if you change something here and you also have to change something there, you could forget/introduce bugs/.... and it is hard to never forget.

So the point is: you aren't eliminating duplication for the sake of it or because someone said it's important. You are doing it because you want to reduce bugs/problems. In your case, it appears that if you change something in a view, you probably won't need to change the exact same line in all of the other views. So you have apparent duplication, not actual duplication.

Another important point is to never rewrite from scratch something that is working now only on the basis of a matter of principle, as Joel said (you could have already heard of him....). So, if your views are working, proceed to improve step by step and don't fall prey to the "single worst strategic mistake that any software company can make".

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.