签入“注释掉的”代码

小开

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.

小开

I absolutely agree that commented out code shouldn't be checked into the repository, that is what source code control is for.

In my experience when a programmer checks in commented out code, it is because he/she is not sure what the right solution is and is happier leaving the alternate solution in the source in the hope that someone else will make that decision.

I find it complicates the code and makes it difficult to read.

I have no problem with checking in half finished code (so you get the benefit of source control) that isn't called by the live system. My problem is with finding sections of commented code with no explanation the dilemma was that resulted in the code being excluded.

小开

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.

小开

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

小开

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.

小开

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.

小开

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.

小开

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.

小开

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.

小开

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.

小开

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.

小开

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.

小开

I think there are better alternatives to commented-out code.

One option which allows this other developer to check in temporary or experimental code is to have per-developer #defines (assuming you're using a language that supports it).

I have worked at places where every coder had their own #define, matching their username, which they could use for things that only they were interested in. Eventually when the code was ready for prime-time, the #if checks were removed.

For instance you might have

#if JOEBLO
// This code is experimental


// ... code goes here ...


#endif // JOEBLO

Regarding Rex M's answer, I think there are sometimes cases when the incremental check-in you want to make (to record progress) does not function well enough to enable for everyone. In such cases, the #define is useful.

Another alternative is to locally use a DVCS like Mercurial to track your changes, then when you get to a stable "ready for the public" state, you push those changes out to the team. Be careful of submitting too much at once, though.

小开

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

小开

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.

小开

In my experience, developer switches are commented out code.

Sometimes, new back-ends are built in parallel, with the activating switchs commented out in source control.

Some bizarre feature we need once on a blue moon but no customer will ever need is often implemented that way. These things usually carry a high risk of security or data integrity bypass so we don't want them active outside of development. Requiring a developer who would use it to uncomment the code first seems to be the easiest way of getting it.

小开

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.

小开

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.

小开

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.

小开

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.

小开

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.

小开

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.

小开

Another reason for checked in commented out code:

You're modifying existing code, and found a subtle bug, one that is easy to overlook, and perhaps might even look correct at first glance. Comment it out, put the fix in its place, and add comments for what is going on, and why it was modified. Check that in, so that your comments on the fix are in the repository.

小开

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.

小开

I think checking-in commented code in a source code control system should be done with extreme caution, especially if the language tags used to comment the code are written by blocks, i.e.:

/* My commented code start here
My commented code line 1
My commented code line 2
*/

Rather than on an individual line basis, like:

// My commented code start here
// My commented code line 1
// My commented code line 2

(you get the idea)

The reason I would use extreme caution is that depending of the technology, you should be very careful about the diff/merge tool you are using. With certain source code control system and certain language, the diff/merge tool can be easily confused. The standard diff/merge of ClearCase for example is notoriously bad for merging .xml files.

If it happens that the commenting blocks lines are not merge properly, presto your code will become active in the system when it shouldn't be. If the code is incomplete and break the build, that is probably the least evil, as you will spot it immediately.

But if the code passes the build, it may become active when it shouldn't be there, and from a CM perspective, that could be a nightmare scenario. QA generally test what should be there, they don't test for code that is not supposed to be there, so your code may end up in production before you know it, and by the time it would be realized the code is there when it shouldn't, the cost in maintenance have multiplied many folds (as the "bug" will be discovered in production or by the customer, worst place or time ever).

小开

If the developer has commented out some code because it is not yet complete, then the correct "source control" way to deal with this would be for that developer to keep it in a separate branch of his own, until that code is ready to check in.

With a DVCS (like git, bazaar, or mercurial) this is dead easy as it requires no changes in the central repository. Otherwise, perhaps you could talk about giving developers their own branches on the server if they are working on specific features that will take them a long enough time (ie, days).

There is nothing wrong with checking in commented out code in some situations, it's just that this is one situation where there may be a better way to do it, so the developer can track changes to his source even though it isn't ready to be checked in to the main repository.

小开

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

小开

"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

If you have a private branch and
You don't have time to make the code compile without errors and
You are going on a long vacation and
You don't trust your VCS, like if you use Visual Source Safe OR.
[EDIT]
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!

小开

I would prefer to see possibly broken, accessible code that isn't being used yet checked in over the same code being completely unavailable. Since all version control software allows some sort of 'working copy' separate from the trunk, it's really a much better idea to use those features instead.

New, non-functional code is fine in the trunk, because it is new. It probably doesn't break anything that already works. If it does break working code, then it should just go in a branch, so that other developers can (if they need to) check that branch out, and see what's broken.

小开

In C++, I use #if 0 for commented code and find it legitimate. Refactoring tools still work on it, so technically it is almost maintained. And, it's distinctly colored by syntax highlighting.

I think such feature would be useful for all:

this code is not operational
it still belongs here
and it must be very visible at this very point, because it is relevant.

Always delete commented code if it has no real comment explaining what is so special about this code and why is it so important, despite being inoperational.

小开

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.

小开

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.