Comment by cbsks

5 years ago

One of my most proud accomplishments was reducing the size of a driver from ~3000 lines of code to ~800. The file was 15 years old and had been modified by many people since. There were conditionals that were impossible to hit, features that had been abandoned years ago, duplicate code, and lots of comments that didn’t match the code anymore. After my changes and a few new tests, the driver had full MCDC code coverage and the code actually matched the device specification!

That is a win for everybody!

My similar story was removing a 10,000 line module that built hundreds of different packets for sending over a mailbox to a wifi module. Each method was almost identical, with the exception of building a slightly different header.

I replaced it with a template that given the structure, built a packet to send it. Less than 1 page of code.

  • Wow, that almost makes it sound like the original author was getting paid per line.

    • Must have. It was a new version of an old driver. The old driver was 9 modules. The new one - 900. Every tiny little thing was another module. It conversed in 802.11 and ethernet etc, so of course it had three (3) copies of non-compatible packet layout declarations, each comprising dozens of modules. And on and on.

      It was like a computer science geek gone mad had figured, "I'll use every decomposition technique I ever heard of, and invent some more, so this is the most computer-sciencey source base in the world".

      Ultimately I rewrote it in 12 C++ base classes and a derived object for each radio card I had to work with.

      1 reply →

There is nothing more satisfying than refactoring code to something nice and condensed, but still very readable. A lot of my personal favorite experiences come from working with trees, since recursive code can be so lean and beautiful. I write a 5ish line function to generate a graphviz file to render by AST once and I still look back on that as beautiful code.

> There were conditionals that were impossible to hit

The number of times I've seen:

  if (my_var == 'some value' || true == false) { ... }

Why do people do this instead of commenting out the code?!

  • Because if you comment out the code you don't benefit any more from the compiler checking that the inner block is still legal, or from your IDE to do the refactoring operations you want. The code would bit rot much faster in a comment (or removed from the code base altogether).

    The thing I really can't understand is why you would compare a boolean condition to `true` instead of checking the condition itself (in other words, writing `false` instead of `true == false`). Also, let me suggest to use `&&`, not `||`. And while we're at that, I would actually write:

        if (false && my_var == 'some value') { ... }
    

    just in case operator `==` can have side effects or relevant computational cost in your language.

  • Just to clarify, what do you mean "instead of commenting out the code"? The above is equivalent to

      if (my_var == 'some value') { ... }
    

    which is not the same as commenting it out entirely.

    • I think it may be a typo, and they meant "&&" instead of "||". That would be the equivalent of "if (false)", thus preventing that block of code from ever executing.

  • Why even comment out? We have version control for a reason

    • Version control is not immediately visible to future developers. You need to know that there is something to look for, particularly if it is a frequently changed file.

      3 replies →

    • I've been at enough companies that have decided to change version control without pulling over history that I have developed a trust issue with version control. It is a great tool when used correctly, but comments are even more resistance to certain kinds of corner cutting.

    • I guess I've been assuming they did that because they wanted to temporarily disable whatever was within the conditional block but, yeah, you're right. Just delete it if you don't want that to run. Either way, that code should never be committed.

  • I have used constructs like this in debugging / exploration phases (especially in languages without good debuggers, when print debugging is unavoidable or just faster). I'd be horribly embarrassed if they got committed. But... the snippet you posted isn't equivalent to a comment; `x or false` is equivalent to `x`. So it's actually equivalent to a commented-out short-circuit

        if (my_var == 'some value' || true == false) { ... } //if (my_var == 'some value') { ... }
    
        if (my_var == 'some value' || true == true) { ... } //if (true) { ... }
    
        if (my_var == 'some value' && true == false) { ... } //if (false) { ... }
    

    That said... I would never use the comparisons `true == true` and `true == false`... that's just silly

    • > that's just silly

      Right? Wouldn't it be more readable to use just 'true' or 'false' instead of the comparisons, or is that not a feature in some languages? I don't understand what might be gained from the extra comparison, besides confusion.

      if (my_conditional || true) { etc }

  • That's not equivalent to commenting out the code block, it's just equivalent to

      if (my_var == 'some value') { ... }

That's awesome. Clearing out cruft without breaking anything or losing functionality is tricky, but man does it feel good.

The tricky side of this is languages that encourage terseness through "clever" syntax.

But that's arguing syntax instead of principle, I think generally "less code" means "fewer expressions to evaluate"

  • +1 to this. Code is fundamentally harder to read than write. If I use clever syntax, my plain-English comments make up the difference for characters saved :)

I'm sure I can reduce the code size of the codebase I inherited by half if I go ham on it, but it's 150K LOC of poorly written JS and PHP (like, really poorly, I could provide The Daily Wtf with a dozen articles I'm sure) and I'd rather spend my time on the rebuild.

Of course, my manager is telling me to keep adding features to the old codebase. Sigh.