Peeve of the moment: People who comment out old code.

schwern on 2005-07-25T22:30:56

I hates nothing more than to find code like this:

# for( my $up = 1;  $up <= $height + 1;  $up++ ) {                     
for( my $up = 1;  $up <= $height;  $up++ ) {

It causes my eyes to do a double take. If my brain made noise it would be that *tick tick* *tick tick* *griiiIIIIiinnndd* that a hard drive makes when trying to work with a bad block. "Its code so I should be reading it but its a comment so I should be ignoring it but its code... but its a comment... and it looks just like the next line of code so its probably a bug... but its a comment... but its code... "

I've been told syntax highlighting browsers help with this, but its not a problem I want to be helped with. And I'd love to see what code looks like after a few months of this sort of style. More comment than code.

Anyhow, it reflects a fundamental lack of trust in your version control system, or a fundamental lack of a version control system. All the more amusing because I received a diff today. It contained this:

-        for( my $up = 1;  $up <= $height + 1;  $up++ ) {                       
+        # for( my $up = 1;  $up <= $height + 1;  $up++ ) {                     
+        for( my $up = 1;  $up <= $height;  $up++ ) {

Yes!

mary.poppins on 2005-07-26T02:45:31

I had this exact same conversation with a coworker just a couple weeks ago. I was reviewing some changes, which included -- yes -- commenting out code.

His argument was that the commented-out code, for a feature we were removing, would probably be needed again soon. He was expecting the feature to be re-added again. I pointed out that:

1) Predicting the future is hard, and every time you're wrong you just clutter up the code
2) The old code still existed in the old versions in CVS

I thought I had convinced him, but the moment I left his cubicle he lost courage and changed his mind. Gah!

for loops

mary.poppins on 2005-07-26T02:52:24

Wouldn't this:

for( my $up = 1; $up = $height; $up++ ) {

be more easily read if it were written as:

for my $up (1 .. $height) {

?

Re:for loops

schwern on 2005-07-26T07:54:57
No, because $height can change inside the loop. The code is from Sub::Uplevel. There's an explainatory comment and everything.

Re:

Aristotle on 2005-07-26T06:03:19

ACK.

I do this a lot, temporarily, when I’m hunting bugs or when I just want to try something else. But as soon as I’ve decided which version I need, the commented crap goes out the window.

What I find particularly jarring is finding entire sections of code or complete routines commented out in someone else’s source. It doesn’t particularly inspire trust in the codebase to see that someone is littering it with his failed experiments…

better than #ifdef

jdavidboyd on 2005-07-26T15:05:46

(this is in C, of course)

I have a co-worker who always changes the code that he fixes by wrapping it in #ifdef tags, like

#ifdef _BUGS_
...old broken code
#else
...new working code
#endif

This drives me crazy, but he won't change, and no one here with authority wants to enforce anything against, this, so I just take them out when I get a chance to work on the affected files.

Re:better than #ifdef

merlyn on 2005-07-26T15:48:52
That's simple. Just go in to the standard Makefile for your build, and add -D_BUGS_.
It'll drive him nuts for hours.

In our non-perl code base

runrig on 2005-07-26T17:58:55

We use an IDE with code-folding. People here often comment out the top level line for a block (which automatically comments out the whole block), copy it, and make a minor change somewhere in the block (or move the block to a different library file), and then leave behind the old code in the commented out block. There are thousands of lines of commented out blocks. I wrote a crufty-comment remover in perl which removes any commented out blocks of 100 lines or more, which in some of the larger libraries saves a few hundred KB of space.

Just ignore it

ziggy on 2005-07-28T16:00:23

I take two steps when taking over some legacy code:

Delete all the comments
Normalize the formatting, including vertical whitespace

It works well enough when you're starting out on an existing codebase. At the end of the day, 3-year old (and incomplete) edit histories aren't that important, and commentary on why something changed into its current state is useless, because you're not likely to see the conditions that triggered the change way back when.

Additionally, focusing on just the code helps you focus on the problem. Usually, projects like this have overly verbose idioms and massively over-engineered complicated logic. Once you start teasing out those idioms and logic, and replace them with something easier to read, the amount of code starts to shrink drastically. (You are testing against a regression test, and using version control, aren't you? ;-)

Interestingly, I picked up this pattern when working on a large FORTRAN codebase, where everyone commented out code and copied the old source into an archive directory before committing a change. When these antipatterns get out of hand, extreme measures are the quickest way to restore your sanity.

Fluff

petdance on 2005-07-28T19:39:51

Commented-out code is one of the things that my fluff program warns on.