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

My view: if developers are working on their own branches, or in their own sandbox area, then they should be able to check in whatever they want. It's when they check code into a shared branch (a feature branch, or a team's branch, or of course MAIN/trunk) that the code should be as pristine as possible (no commented out code, no more FIXMEs, etc).

Answer 2

Clearly, the developer who is checking in commented-out code should be working in a separate branch, merging in changes from the trunk branch as neccessary.

It is up to the VCS system to assist the developer in this workflow (git is one excellent VCS system that works with this very nicely).

Answer 3

I don't know - I always comment out the original lines before I make changes - it helps me revert them back if I change my mind. And yes I do check them in.

I do however strip out any old commented code from the previous check-in.

I know I could look at the diff logs to see what's changed but it's a pain - it's nice to see the last changes right there in the code.

Answer 4

When you need to add a small feature or bug fix like NOW, within the next 3 minutes and you have to fix a file you have some half developed code on I'd say it's ok, practical needs rule over pragmatic ideals on the battlefield.

Answer 5

This shows a fundamental difference in two schools of thought: Those who only check in working code that they are satisfied with and feel is worthy of saving, and those who check in their work so the revision control is there to backstop them against data loss.

I'd charactarize the latter as "those who like to use their revision control system as a poor-man's tape backup", but that would be tipping my hand as to which camp I'm in. :-)

My guess is that you are of the "good code" camp, and he is of the "working code" camp.

[EDIT]

From the comments, yes I guessed right.

As I said, I'm with you, but as near as I can tell this is a minority opinion, both here on stackoverflow and where I work. As such, I don't think you can really enshrine it in your development standards as the only way to operate. Not if you want the standards followed anyway. One thing a good leader knows is to never give an order they know won't be followed.

btw: Good editors will help with keeping old versions. For example, in Emacs I set kept-old-versions and kept-old-versions to 10, which has it keep around the last 10 saves of my files. You might look into that as a way to help your argument against the revision-control-as-backup crowd. However, you won't ever win the argument.

Answer 6

The idea of allowing source-control history to illustrate the "old way" of doing something rather than commenting it out and checking in the commenting-out along with an explanation is a good idea in theory.

In the real world, however, nobody ever looks at source control history on the files they are working on unless it is part of an official review process of some sort (done only periodically), or if something doesn't work, and the developer can't figure out why.

Even then, looking back more than about 3 versions basically never happens.

Partially, this is because source-control systems don't make this sort of casual review easy. Usually you have to check out an old version or diff against an old version, you just see two versions, and there's no good concise view of what changed that can give you an at-a-glance idea of what changed.

Partially, it is the combination of human nature and the needs of the team. If I have to fix something, and I can fix it in a few hours, I'm not likely to spend an hour investigating old versions of the code that haven't been "live" in a month (which, with each developer checking in often, means back many revisions), unless I happen to know that there's something in there (such as if I remember a discussion about changing something related to what I'm doing now).

If the code is deleted and checked back in, then, for all intents and purposes (except for the limited purpose of a complete roll-back) it ceases to exist. Yes, it is there for backup purposes, but without a person in the role of code librarian, it is going to get lost.

My source control tree on my current project is about 10 weeks old, on a team of only about 4 engineers, and there are about 200 committed change lists. I know that my team does not do as good of a job as it should of checking in as soon as there is something solid and ready to go. That makes it pretty rough to rely on reading the code history for every part of the code to catch every important change.

Right now, I'm working on a project in initial development mode, which is very different from a project in a maintenance mode. Many of the same tools are used in both environments, but the needs differ quite a bit. For example, often there is a task that requires two or more engineers to work somewhat closely together to build something (say a client and a server of some sort).

If I'm writing the server, I might write up the code for the draft interface that the client will use and check it in completely non-functional, so that the engineer writing the client can update. This is because we have the policy that says that the only way to send code from one engineer to another is through the source control system.

If the task is going to take long enough, it would be worth creating a branch perhaps for the two of us to work on (though that is against policy in my organization -- engineers and individual team leads don't have the necessary permissions on the source-control server). Ultimately, its a trade-off, which is why we try not to institute too many "always" or "never" policies.

I would probably respond to such a no-commented-code-ever policy by saying that it was a bit naive. Well-intentioned, perhaps, but ultimately unlikely to achieve its purpose.

Though seeing this post is going to make be go back through the code I checked in last week and remove the commented-out portion that was both never final (though it worked) and also never likely to be desired again.