Checking in of “commented out” code [closed]

Question

Ok, here is something that has caused some friction at my current job and I really didn't expect it to. Organized in house software development is a new concept here and I have drawn up a first draft of some coding guidelines.

I have proposed that "commented out" code should never be checked into the repository. The reason I have stated this is that the repository maintains a full history of the files. If you are removing the functional code then remove it altogether. The repository keeps your changes so it is easy to see what was changed.

This has caused some friction in that another developer believes that taking this route is too restrictive. This developer would like to be able to comment out some code that he is working on but is incomplete. This code then would never have been checked in before and then not saved anywhere. We are going to be using TFS so I suggested that shelving the changes would be the most correct solution. It was not accepted however because he would like to be able to checkin partial changes that may or may not be deployed.

We want to eventually get to a point where we are taking full advantage of Continuous Integration and automatically deploying to a development web server. Currently there is no development version of web servers or database servers but that will all be changed soon.

Anyway, what are your thoughts? Do you believe that "commented out" code is useful to have in the repository?

I'm very interested to hear from others on this topic.

Edit: For clarity sake, we don't use private branches. If we did then I'd say do what you want with your private branch but don't ever merge commented out code with the trunk or any shared branches.

Edit: There is no valid reason we don't use private or per user branches. It's not a concept I disagree with. We just haven't set it up that way yet. Perhaps that is the eventual middle ground. For now we use TFS shelving.

Answer 1

Just echoing the chorus. Discourage this at all costs. It makes the code harder to read and leaves folks wondering whats good/bad about that code that isnt even part the application at the present time. You can always find changes by comparing revisions. If there was some major surgery and code was commented out en-masse the dev should have noted it in the revision notes on checkin/merge.

incomplete/experimental code should be in a branch to be developed to completion. the head/trunk should be the mainline that always compiles and is whats shipping. once the experimental branch is complete it/accepted it should be merged into the head/mainline. There is even an IEEE standard (IEEE 1042) describing this if you need support documentation.

Answer 2

In general, checking in commented-out code is wrong as it creates confusion amongst those who are not the original author and need to read or amend the code. In any event, the original author often ends up confused about the code after 3 months have passed.

I espouse the belief that the code belongs to the company, or team, and that it is your responsibility to make things easy for your peers. Checking in commented-out code without also adding a comment about why it is being retained is tantamount to saying:

I don't mind if you end up all confused over why this stuff is here. My needs are more important that yours which is why I have done this. I do not feel any need to justify to you, or anyone else, why I have done this.

For me commented-out code is normally seen as a sign of disrespect from a less that thoughtful co-worker.

Answer 3

There may be others with different experiences, but in mine checking in half-finished code is a horrible idea, period.

Here are the principles I have learned and try to follow:

• Check in often - at least once, but preferably many times per day
• Only check in complete functionality
• If the first and second conflict (e.g. it takes more than a day to make the functionality work) then the task is too large - break it into smaller completable tasks.

This means:

• Commented-out code should never be checked in since it is not functional
• Commenting is not a valid archival strategy, so whether it's code yet-to-be-finished or code that's being retired, commenting and checking in doesn't make any sense.

So in summary, NO! If the code is not ready to go to the next stage (whichever that is for you: IntTest/QA/UAT/PreProd/Prod), it should not be committed to a trunk or multi-developer branch. Period.

Edit: After reading the other answers and comments, I'll add that I don't think it's necessarily a good idea to ban commented code (not sure how you'd enforce that anyway). What I will say is that you should get everyone on your team to buy in to the philosophy I described above. The team I work on embraces it wholeheartedly. As a result, source control is a frictonless team-member, one that helps us get our job done.

People who don't embrace that philosophy usually cause broken windows and are often frustrated by source control. They see it as a necessary evil at best, and something to avoid at worst; which leads to infrequent checkins, which means changesets are huge and hard to merge, which compounds frustration, makes checkins something to avoid even more, etc. This is ultimately an attitude thing, not really a process thing. It's easy to put up mental barriers against it; it's easy to find reasons why it won't work, just like it's easy to find reasons not to diet if you don't really want to. But when people do want to do it and are committed to changing their habits, the results are dramatic. The burden is on you to sell it effectively.

Answer 4

I think "Never" is too strong a rule. I'd vote to leave some personal leeway around whether people check commented code in to the repository. The ultimate goal should be coder productivity, not "a pristine repository."

To balance that laxness out, make sure everyone knows that commented out code has an expiration date. Anyone is allowed to delete the commented code if it's been around for a full week and never been active. (Replace "a week" with whatever feels right to you.) That way, you reserve the right to kill clutter when you see it, without interfering too directly with people's personal styles.

Answer 5

I broadly agree with the principle that commented out code shouldn't be checked in. The source control system is a shared resource, and your colleague is, to an extent, using it as his personal scratch pad. This isn't very considerate to the other users, especially if you subscribe to the idea of shared ownership of the codebase.

The next developer to see that commented-out code would have no idea that it's a work in progress. Is he free to change it? Is it dead code? He doesn't know.

If your colleague's change isn't in a state where it can be checked in, he needs to finish it, and/or learn to make smaller, incremental changes.

"Checking in partial changes that may or may not be deployed" - presumably that also means may or may not be tested? That's a slippery slope to a very ropey code base.

Answer 6

It depends. If it's being left there for purposes of illustration, maybe. It could possibly be useful during refactoring. Otherwise, and generally, no. Also, commenting out unfinished code is bound to be both failure-prone and a time sink. Better he break the code into smaller pieces and check them in when they work.