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

There is clearly a tension between 1) checking in early, and 2) always maintaining the repository in a working state. If you have more than a few developers, the latter is going to take increasing precedence, because you can't have one developer crapping all over everyone else for his own personal workflow. That said, you shouldn't underestimate the value of the first guideline. Developers use all different kinds of mental fenceposts, and individualized workflows are one way that great developers squeeze out those extra Xs. As a manager your job not to try to understand all these nuances—which you will fail at unless you are a genius and all your developers are idiots—but rather enable your developers to be the best they can be through their own decision-making.

You mention in the comment that you don't use private branches. My question for you is why not? Okay, I don't know anything about TFS, so maybe there are good reasons. However after using git for a year, I've gotta say that a good DVCS totally diffuses this tension. There are cases where I find commenting out code to be useful as I'm building a replacement, but I will lose sleep over it if I'm inflicting it on others. Being able to branch locally means I can keep meaningful commits for my individual process without having to worry about (or even notify) downstream developers of temporary messes.

Answer 2

"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."

Answer 3

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.

Answer 4

I think commented out code is considered to be "waste".

I am assuming you are working in a team environment. If you are working on your own, and you comment out code with a 'todo' and you will come back to it then that is different. But in a team environment you can safely assume once commented out code is checked in it is there to stay and it will most likely cause more pain than satisfaction.

If you are doing peer code reviews then it might answer your question. If another developer reviews your code and says "why is there this commented out code that is trying to do 'blah'" then your code has failed the code review and you shouldn't be checking it in anyway.

Commented out code will just raise questions with other developers - therefore wasting time and energy.

You need to ask the question "why" the code is commented out. Some suggestions:

If you are commenting out code because you are "unsure of business rules" then you probably have an issue with "scope creep" - best not to dirty your code base with requirements that "would be nice to have but we don't have time to implement" - keep it clean with clear code and tests around what is actually there.

If you are commenting out code because you are "not sure if it is the best way to do it" then have your code peer reviewed! Times are changing, you will look at code you write today in 2 years and think it's horrible! But you can't go around commenting out bits that you 'know' can be done better but you just can't find a way right now. Let whoever maintains the codebase long term determine whether there is a better way - just get the code written, tested and working and move on.

If you are commenting out code because "something doesn't work" then FIX IT! A common scenario is "broken tests" or "todo's". If you have these, you will save yourseslf a lot of time by either fixing them or just getting rid of them. If they can be "broken" for a period of time, they can most likely be broken forever.

All of these potential scenarios (and ones I haven't mentioned here) are wasted time and effort. Commented out code might seem like a small issue but could be an indicator of a bigger issue in your team.

Answer 5

A nice compromise is to write a little tool that dumps your checked out/modified files to a network backup drive. That way, you can modify til your heart's content and have your work backed up, but you never have to check in experimental or unfinished code.

Answer 6

I think that checking in commented out code should be valid as, just because the new change passed tests it may be more helpful to see what was there before and see if the new change is really an improvement.

If I have to go back several versions to see an earlier change that now leads to a performance hit then that would be very annoying.

Sometimes the commented out code is a good history, but, put dates as to when the code was commented out. Later, someone that is working near there can just delete the commented out code as it has been proven not to be needed.

It would also be good to know who commented out that code so that if some rationale is needed then they can be asked.

I prefer to write new functionality, ensure the unit tests pass, check it in, then allow others to use it and see how it works.