Hacker News new | ask | show | jobs
by sozin 1023 days ago
I think the author gets it wrong.

The cost of DRY (Don't Repeat Yourself)-ing up your code can be high, in that it increases the coupling of your code, and potentially lowers its cohesion.

Consider function def foo(a: int), called from call sites C1 and C2. Eventually C1 wants something out of foo() that it doesn't offer, but, critically, something that C2 _doesn't care about or need_. The author of foo() adds a new default argument: def foo(a: int, b:int = 0), and then there is a conditional block in foo() that deals just with this new b argument.

You've now potentially broken callsite C2, by exposing it to changes that it doesn't care about it. Put another way: you should only deduplicate the code of _all_ the call sites will _always_ change for the same reason. Otherwise, you're lowering code quality of the code by increasing coupling and lowering cohesion. Copy and pasting the code in this case makes sense, because C1 and C2 both have entirely different needs out of foo(). Overtime, foo() will accumulate more and more default arguments as the author stridently attempts to keep everything DRY, and the overall code base becomes more and more fragile.
1 comments
jackcviers3 1023 days ago
So you make:

    // new foo block 
    // using b:int stuff, 
    // calling fooInternalsX,
    // fooInternalsY, etc.
    // for common functionality
    foo(a: int, b:int) = ...
and

    // stays the same
    // except replacing
    // common functionality with
    // calls to fooInternalsX, 
    // fooInternalsY, etc. 
    foo(a: int) = ...
and enough

    private fooInternalsX(a:Int) = ...
    private fooInternalsY(a: Int) = ...
methods to cover the common functionality.

Your code is still DRY, and you are using polymorphism (foos of different type signatures) instead of if/else, the behavior of foo(int) doesn't change, so you don't require additional tests for foo(int), the fooInternals<X,Y,Z> aren't public, and you have now added tests for foo(int, int). You aren't paying any additional costs in terms of maintenance. You aren't increasing behavioral risk at C2 for calling foo(int). You are only paying more for foo(int, int), and those are costs that you would have to pay regardless of if foo(int, int) literally duplicated the body of foo(int) for common pieces or refactored the common pieces out. You save cost for maintaining both foo(int) and foo(int, int) if the common pieces need to change, as you are adding tests for the behavioral changes to both foo(int) and foo(int,int) tests, but are only making a single change in the common code.

Also, when doing this, the abstraction is the original foo(int), not the new, additional foo(int, int). Abstraction is the assumption of some parameterized behavior via hard-coding. Here, the new, additional parameterized behavior introduced by the second b:int parameter is abstracted away in the original foo(int), not in the new foo(int, int). That doesn't make the original foo(int) abstraction wrong, because it is used in at least one call site (C2).

Only when all call sites must change to accommodate something that a new parameter allows through more than one change-set can you begin to call an abstraction wrong. Otherwise, it is a simple bug that was fixed by a single change-set.

link