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

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.

Answer 2

Perhaps the real question here is whether developers should be allowed to check in incomplete code?

This practice would seem to be contradictory to your stated goal of implementing continuous integration.

Answer 3

Repositories are backup of code. If I am working on code but it is not completed, why not comment it out and check it in at the end of the day. That way if my hard drive dies overnight I will not have lost any work. I can check out the code in the morning uncomment it and keep on going.

The only reason I would comment it out is because I would not want to break the overnight build.

Answer 4

"Scar Tissue" is what I call commented-out code. In the days before the widespread use of version-control systems, Code Monkeys would leave commented out code in the file in case they needed to revert functionality.

The only time it is acceptable to check-in "scar tissue" is

1. If you have a private branch and
2. You don't have time to make the code compile without errors and
3. You are going on a long vacation and
4. You don't trust your VCS, like if you use Visual Source Safe OR.
   [EDIT]
5. You have a subtle bug that might be reintroduced if the incorrect code isn't left in as a reminder. (good point from other answers).

There is almost no excuse for #4 because there are plenty of freely available and robust VCS systems around, Git being the best example.

Otherwise, just let the VCS be your archive and code distributor. If another developer wants to look at your code, email him the diffs and let him apply the stuff he wants directly. In any event, the merge doesn't care why or how the coding of two files diverged.

Because it is code, scar-tissue can be more distracting even than a well-written comment. By its very nature as code, you make the maintenance programmer expend mental cpu-cycles figuring out if the scar-tissue has anything to do with his changes. It doesn't matter whether the scar is a week old or 10 years old, leaving scar-tissue in code imposes a burden upon those who must decypher the code afterwords.

[EDIT] I'd add that there are two major scenarios that need to be distinguished:

• private development, either coding a personal project or checking in to a private branch
• Maintenance development, where the code being checked in is intended to be put into production.

Just say "NO" to scar-tissue!

Answer 5

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.

Answer 6

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.