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.

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.

Really the only time a future developer will care about the old code is when there's a bug in the current code. (Or maybe an edge case like a request to "make it work like it used to in v1.0").

Looking at history via "blame" is useful to see why a bug was introduced (was it fixing another bug, and if so, is your fix going to break that bug again?), and how long it's existed for.

Leaving old code commented out doesn't help either with of those things. Unless maybe it's accompanied by lots of comments and date stamps, in which case you've not only re-invented a very crappy, half-baked version control system, but also made the code hard to read and work with.

git blame only tells you information about the last time a line was changed. If the code is commented out, I can quickly find the version said code was commented out.

Git blame also doesn't help when the history gets truncated for performance reasons.

I kind of agree; I've never looked in a file's history to see if something had been solved previously. I don't recall looking in history more than a week back to restore some deleted code either. In practice, I just rewrite code instead, also with the mindset that I'm a better programmer today than I was yesterday, and that in my head at least, writing that bit of code is trivial.

But overall, if commented code ends up in version control, it probably could have been removed.

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.