Checking in of "commented out" code [closed]

Question

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.

"Never" is rarely a good word to use in guidelines.

Your colleague has a great example of when it might be appropriate to check in code that is commented out: When it is incomplete and might break the application if checked in while active.

For the most part, commenting out dead code is unnecessary in a well-managed change-controlled system. But, not all commented code is "dead."

One case where I leave commented out code:

// This approach doesn't work
// Blah, blah, blah

when that is the obvious approach to the problem but it contains some subtle flaw. Sure, the repository would have it but the repository wouldn't warn anyone in the future not to go down that road.

Commented out code should never be checked-in for the purpose of maintaining history. That is the point of source control.

People are talking a lot of ideals here. Maybe unlike everyone else, I have to work on multiple projects with multiple interruptions with the "real world" ocassionally interrupted my workday.

Sometimes, the reality is, I have to check-in partially complete code. It's either risk losing the code or check-in incomplete code. I can't always afford to "finish" a task, no matter how small. But I will not disconnect my laptop from the network without checking-in all code.

If necessary, I will create my own working branch to commit partial changes.

I would certainly discourage, strongly, ever checking in commented-out code. I would not, however, absolutely ban it. Sometimes (if rarely) it is appropriate to check commented-out code into source control. Saying "never do that" is too restrictive.

I think we all agree with these points:

• Never check dead code into source control
• Never check broken (non-functioning) code into source control, at least never to trunk and only very rarely to a private branch, YMMV
• If you have temporarily commented something out or broken something for debugging purposes, don't check the code in until you restore the code to its correct form

Some of are saying there are other categories, such as temporarily removed code, or an incremental but incomplete improvement that includes a small amount of commented-out code as documentation of what comes next, or a very short (ideally 1 line) snippet of commented out code showing something that should never be re-added. Commented-out code should ALWAYS be accompanied by a comment that says why it is commented out (and not just deleted) and gives the expected lifetime of the commented-out code. For example, "The following code does more harm than good, so is commented out, but needs to be replaced before release XXX."

A comment like the above is appropriate if you are delivering a hotfix to stop a customer's bleeding and you don't have the immediate opportunity to find the ultimate fix. After delivering the hotfix, the commented-out code is a reminder that you still have something that needs fixing.

When do I check in commented-out code? One example is when I am tentatively removing something that I think there's a high probability will have to be re-added in the near future, in some form. The commented-out code is there to serve as a direct reminder that this is incomplete. Sure, the old version is in source control and you could just use a FIXME comment as a flag that something more is needed. However, sometimes (if not often) code is the better comment.

Also, when a bug is fixed by removing one line (or more rarely, two lines) of code, I'll sometimes just comment out the line with a comment to never re-enable that code with a reason why. This sort of comment is clear, direct, and concise.

Rex M said: 1) Only check in complete functionality, 2) [If] the task is too large - break it into smaller completable tasks.

In response: Yes, this is the ideal. Sometimes neither option is achievable when you are working on production code and have an immediate, critical problem to fix. Sometimes to complete a task, you need to put a version of code in the field for a while. This is especially true for data gathering code changes when you're trying to find the root cause of a problem.

For the specific instance being asked about in the more general question ... as long as the developer is checking commented-out code into a private branch that no-one will see but that developer (and perhaps someone the developer is collaborating with), it does little harm. But that developer should (almost) never deliver such code into trunk or an equivalent. Trunk should always build and should always function. Delivering unfinished code to trunk is almost always a very bad idea. If you let a developer check unfinished or temporary code into a private branch, then you have to rely on the developer to not forget to scrub the code before delivering into trunk.

To clarify in response to comments to other answers, if code is commented out and checked in, my expectation that the code will function if uncommented drops with the length of time the code has been commented out. Obviously, refactoring tools will not always include comments in their refactoring. Almost always, if I put commented-out code into production, the code is there to serve as a refined comment, something more specific than prose, that something needs to be done there. It is not something that should have a long life.

Finally, if you can find commented-out code in every source file, then something is wrong. Delivering commented-out code into trunk for any reason should be a rare event. If this occurs often, then it becomes clutter and loses its value.

I think never is too strong a condition.

I tend to comment out, checkin, run the tests, have a think and then remove comments after the next release.