Hacker News Reader

Comment by chairleader | Hacker News Reader

This is extremely validating to read. How many times have I battled with DRYists over which solution is "better."

I've happened upon the pattern of code growth described here after years of encountering and resolving pain points in code, often as the maintainer. DRYness for DRYness sake might feel satisfying when writing the code, but when changes need to be made, it often scatters constraints and requirements throughout the codebase, making any change an unestimatable mess of sneaky traps. Try writing your boilerplate initialization code straight some time... no metaprogramming, no helper functions, just get your configuration, create your objects and wire them together. It is oddly liberating to have your bootstrap code flat and unmagical.

View on HN

> DRYness for DRYness sake might feel satisfying when writing the code, but when changes need to be made, it often scatters constraints and requirements throughout the codebase, making any change an unestimatable mess of sneaky traps.

This also applies to "one-liners" when writing code. For example in ruby:

Model.where(...).foo { ... }.flatten.bar { ... }.join(...)

Looks nice and all, but it can be confusing when you need to fix a bug that you aren't sure of the source (foo or bar? maybe we shouldn't flatten yet? maybe we should join sooner?). It's much better, IMHO, to write more lines of code here so that you can easily test each step manually when a bug occurs.

I'm not addressing your overall point, but one thing I learned about recently that could help in debugging those Ruby one-liners is the `tap` method. You can stick it in the chain and set a breakpoint.

http://ruby-doc.org/core-2.2.3/Object.html#method-i-tap

Further off-topic, but I almost exclusively see tap used as some kind of clever way around a return line, e.g. instead of:

    def build_hash_or_whatever
      hash = Hash.new
      hash[:foo] = some_generating_method
      hash[:bar] = "some value" if whatever?
      # More hash construction nonsense
      hash
    end

With tap it is:

    def build_hash_or_whatever
      Hash.new.tap do |hash|
        hash[:foo] = some_generating_method
        hash[:bar] = "some value" if whatever?
        # More hash construction nonsense
      end
    end

I think the tap-form is significantly worse and I don't understand the aversion to a single line that clearly shows what you are returning.

Changing code to enable debugging is barbaric. Tooling should be able do that. Or maybe I'm spoiled, lol.

Couldn't agree more. Writing a single line of code that can fail multiple ways (especially the same way on multiple parts) is one of my constant source of annoyances. Not from a DRY perspective, but from your code has a bug, but I can't give you enough detail to immediately fix it because it could be one of these X reasons. Especially on production systems, these become the sorts of issues where the developer makes some temporary fixes to the code, then has wait to observe the bug again to work out which line is broken.

Funny you mention it, I'm also a bit anti-chaining for the same reasons.

Do we need a cute name to describe the practice of optimizing code for maintainability? "Craftsmanship" speaks to the building process, what should we call it when we make code that is understandable and straightforward to change?

Your point seems to be more about using chains of set operators rather than the fact that it is on one line.

I am a C# guy and I find Linq which is the equivalent to this insanely useful and I often write code as above. The tax on this is the rest of the team need to be up to speed in reading such code. If they are then what you gain is use of a bunch of predictable operators rather than loops with counters, state, etc... that could have subtle bugs.

Using code like this is no different to writing SQL that does joins and aggregates in a stored procedure and calling it from your code, something I also thing isn't a bad thing.

Debugging can be done by splitting it into lines, or using a decent enough debugger. Or making it pure and writing unit tests or property-based tests to cover it.

> Your point seems to be more about using chains of set operators rather than the fact that it is on one line.

It isn't, though. My point is that sticking chains on one line makes it difficult to read as well as debug the various bits and pieces that all rest on a single line.

Consider the task of debugging any arbitrary error where you have little more than a line number to go by. You now need to refactor the line just to find out which method invocation is causing the error.

There are also readability issues with lines like these.

> Consider the task of debugging any arbitrary error where you have little more than a line number to go by. You now need to refactor the line just to find out which method invocation is causing the error.

That's a tooling issue. The right thing is to enhance the stack trace to give more than a line number, and the debugger to allow a more specific breakpoint.

> There are also readability issues with lines like these.

Maybe, but the biggest readability issue by far is when a class or function spills beyond a single screen. Avoiding that is worth a lot of cramped lines.

At this point this is boiling down a subjective argument on readability and tradeoffs in tooling, so no point in continuing. Have a great day!

Ok so you are happy with this?

    Model
        .where(...)
        .foo { ... }
        .flatten
        .bar { ... }
        .join(...)

In Visual studio it doesn't really make a difference from a debugging perspective. However in other debugging tools perhaps it does, and in that case I would tend to agree. But then this is a minor point, something that can be covered with a coding standard and linting the source.

Actually, yes. That's exactly what I'm advocating. Whether they are separate assignments or just chains broken up into multiple lines, the end is the same as far as what I'm talking about.

> It isn't, though. My point is that sticking chains on one line makes it difficult to read as well as debug the various bits and pieces that all rest on a single line.

The thing is, if you want to split the line into multiple lines you have to introduce local variables all over. I actually find this harder and more cumbersome to read because if you are reading the rest of the function, you want to know if these are used anywhere (assuming imperative language). If you have everything on a single line, then you know that the intermediate values are not used anywhere else.

I agree that the line number thing is helpful though. However, if you realize you have a bug you'll likely have to reproduce it anyway, and at that time you can introduce local variables just to see where you went wrong.

Perhaps in the end this is just subjective? This style is very common in functional programming (which I prefer) so maybe that's why I like it and hate having more local variables to keep track of.

Whether you need to introduce variables is dependent on the language. Some allow for chains to continue onto a newline, while others won't.

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.

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.

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.

Reply on news.ycombinator.com