Commented-out code eradication with Sonar
There have been numerous debates around commented-out lines of code (line or block of code that was commented out at some point) and whether they should be left in the code or taken out. The outcome of those debates is almost systematically that they should be taken out sooner rather than later : in the Sonar Team, we generally consider than later means after code check in.
Here are the main reasons why old commented-out code is an abomination :
- It always raises more questions than it gives answers
- Everybody will forget very quickly how relevant the commented code is
- This is distraction when going down the code as it stops the flow of eyes
- It is a bad SCM engine : Subversion, CVS and Git are really more trustworthy !
- The simple fact of understanding why code was commented out in the first place can take a lot of time
But the worst of all is in my opinion the fact that commented-out code appeals commented-out code, similarly to The Broken Windows Syndrome ! All this was reinforced by Uncle Bob a few months back in his Clean Code Tip of the Week #7.
There are less discussions about how to tackle this. At least, common sense rules can help to overcome the fear of suppressing such commented-out code :
- Rule 0: Commenting out code is ok, commiting commented-out code is not
- Rule 1: Don’t check in modules that have commented-code
- Rule 2: If you check out a module that has commented-code, delete that code
This pragmatic approach is fine and when adopted the number of commented LOCs is going to decrease throughout time as long as everybody in the team respects the rules. But how to make sure it is the case ? How to measure the current level of commented-out code ? And what’s about the dispersion occurring every time a developer hits some of it ?
Sonar in its version 1.11 has become the ultimate tool to measure and hunt down the commented-out code in the same way it is used to track down potential bugs, complex methods, untested code, duplicated code…
The hunt can start on the project dashboard where the total number of commented-out lines of code is displayed in the “Comments” widget.
When you click on this indicator enables a drill down from project to source code to view the lines of dead code.
To develop this functionality, we have put in place a pretty simple pattern recognition engine that finds commented-out LOCs. For Java language, there are 5 patterns and every time a comment line matches one of those, the probability it used to be a LOC increases. Once a threshold is reached, the comment line is flagged as a commented-out LOC and reported as such in Sonar.
Uncle Bob, if you read this article here is a special message for you : “Clean Code” is really considered as a bible within the Sonar team, and it has directly inspired this new feature. You should have a look in Fitness as there currently are 46 commented-out LOCs in the project :-)
Really nice addition!
However, could you be a little more precise about the pattern used to find the commented code?
Let take the Fitness example you linked here. In the “Parse” class (see permalink at the end of my comment), there are 17 lines commented. However, I count 18 on my side:
1 line: #48
17 lines: #146 to #162
Why is there this difference?
Do you also plan to hightlight the commented code in the code viewer? That would be great :)
Anyway, continue your good job on this marvelous tool!
Romain
Permalink to the Parse class code.
By Romain on November 26, 2009 at 10:50 am
I let Freddy answer to your first question.
The good news is that next release 1.12 introduces the code colorization. It’s a great way to easily find commented-out lines (see this screenshot).
By Simon Brandhof on November 26, 2009 at 11:44 am
Before getting too wrapped up in calling Clean Code a “bible,” you should look at what Bob Martin actually does. After the explanation about the dangers of leaving in commented code, he does a long refactoring exercise. Look at the final output of that exercise. What does it contain? Commented out code. (p. 374 et seq.)
It’s the wrong book to use as a bible. Beck’s Implementation Patterns is a much better guide and internally consistent.
By Anonymous1 on November 27, 2009 at 2:38 am
Hi Romain, the difference is due to line 146 which doesn’t match enough patterns :
“public static String unescape(String s)”
In this line we only detect two java keywords and the detection threshold is not reached. For the next version of this commented-out LOC recognizer, I’m thinking about a way to increase/decrease the probability of a line according to the probabilities of the surrounding lines.
Freddy
By Freddy Mallet on November 27, 2009 at 9:58 am
Thanks for the explanation Freddy !
Maybe if the closing bracket was on the same line (i.e. having “// public static String unescape(String s) {“),it would have been detected as a commented LoC…
It was just for curiosity. We are informed about the commented LoC anyway!
By Romain on November 27, 2009 at 12:09 pm
Your intuition is absolutely correct Romain !!
By Freddy Mallet on November 27, 2009 at 5:38 pm
Thank you for this article! Well said. We had a discussion at work about this and I firmly suggested to take it out since it’s just confusing plus you’ll see the changes anyway in SVN (it’s what we’re using).
By Rommel Garcia on December 18, 2009 at 1:08 am