3 ms·

One of the earliest experiences I had as a professional software developer was taking over an app that had originally been developed by someone with 30 years of

One of the earliest experiences I had as a professional software developer was taking over an app that had originally been developed by someone with 30 years of experience. He had looked at the problem domain, said "these things are all similar", and so had constructed his code so that every one of those things shared a huge amount of their code paths. It worked for the two, three things he actually coded, but as soon as I and another dev started to add more (around 12 total I think), it got hackier and hackier. Because the conditionals needs to distinguish the two or three he had originally handled were easy to ignore, but the conditionals needed to distinguish the 12 were littered all over the place, with no real predictivity to them. We attempted to isolate it as best we could, pulling out into new methods and such, but it was bug whack-a-mole; we reliably were finding a new bug within a week of each fix we made, nevermind any we introduced with new features, because trying to isolate and fix one of the things invariably broke one of the others. So, finally, I took two days and rewrote the shared logic of the application (only around ~10k lines). I borrowed liberally from the existing code, but basically isolated each thing entirely from the others. They still had the same structure, same interface, but rather than all inheriting the same implementation, they all had their own copy of the implementation, which in turn meant each one was far simpler, and could be tweaked in isolation. That took about two weeks of use to flush out the bulk of the bugs. And after that, our mean time to failure kept improving; we went with maybe one bug the third week, then one bug a month for the next two months, then it essentially stabilized; it was only in introducing new features that we'd occasionally find/introduce new bugs. That whole experience taught me not to worry about DRY until you actually have done the work. DRY is to ease maintenance and future development; it should not be a consideration for that initial implementation of a given feature. Because things that look similar enough to share functionality between often aren't once you become more familiar with the domain. If they turn out to be related enough to share functionality, it was trivial to not repeat yourself (copy/paste), and it's trivial to refactor. If they turn out not to share functionality, the effort to try and make them share code was higher than if you'd done them separately, and the amount of effort to pull them apart is huge.

JamesBarney 11y ago
It seems to me what you're describing was a deep and wide inheritance tree. I hate those too. In my experience they have always been very brittle, and lead to confusing code. I'm a huge fan of the recent movement to prefer composition over inheritance.
- lostcolony 11y ago
  I don't remember the particulars. But they're not that important to the nature of the issue (though recognizing them as a bad smell is) - any time you have a reused bit of code (be it via inheritance, composition, or even just a function), you run the risk of that kind of complexity. And this can be hard to spot; a function that takes an argument that it then uses as a conditional in a few places can hide a lot of complexity. That is, if I have a (pseudocode) function foo(bar) { //C //D //D //C //D //C } where //D indicates a line that does not change its behavior based on the passed in bar, and //C indicates a line that does (and in complex ways; if bar is of type bar1 or bar3 do X, else if it's of type bar 5 do Y, else do Z), the code has a lot of interrelated complexities, even though it's DRY. It's better to have foo(bar) { switch(bar) case(...) { foo1(bar) } //etc } so that the implementations are completely different, that even though there will be copied code between foo1, foo2, etc, it's clear what is happening, and you change any bit of foo1, foo2, etc, without fear of it breaking something else (whereas in the original, changing any of those lines marked //C could break unrelated functionality). The issues with the first, and the improved clarity and reduced coupling with the latter, holds true regardless of whether the code is in an inherited object (where the first function would translate to a foo implemented in the superclass, with the //C lines being calls to abstract methods, that are then implemented in the subclasses, and the latter function, foo would be an abstract method itself, and all subclasses implementing their own version, that is partially identical to each others'), or via composition.

JamesBarney 11y ago

It seems to me what you're describing was a deep and wide inheritance tree. I hate those too. In my experience they have always been very brittle, and lead to confusing code. I'm a huge fan of the recent movement to prefer composition over inheritance.

lostcolony 11y ago
I don't remember the particulars. But they're not that important to the nature of the issue (though recognizing them as a bad smell is) - any time you have a reused bit of code (be it via inheritance, composition, or even just a function), you run the risk of that kind of complexity. And this can be hard to spot; a function that takes an argument that it then uses as a conditional in a few places can hide a lot of complexity. That is, if I have a (pseudocode) function foo(bar) { //C //D //D //C //D //C } where //D indicates a line that does not change its behavior based on the passed in bar, and //C indicates a line that does (and in complex ways; if bar is of type bar1 or bar3 do X, else if it's of type bar 5 do Y, else do Z), the code has a lot of interrelated complexities, even though it's DRY. It's better to have foo(bar) { switch(bar) case(...) { foo1(bar) } //etc } so that the implementations are completely different, that even though there will be copied code between foo1, foo2, etc, it's clear what is happening, and you change any bit of foo1, foo2, etc, without fear of it breaking something else (whereas in the original, changing any of those lines marked //C could break unrelated functionality). The issues with the first, and the improved clarity and reduced coupling with the latter, holds true regardless of whether the code is in an inherited object (where the first function would translate to a foo implemented in the superclass, with the //C lines being calls to abstract methods, that are then implemented in the subclasses, and the latter function, foo would be an abstract method itself, and all subclasses implementing their own version, that is partially identical to each others'), or via composition.