31

It bothers me to see commented out code and it seems often-times a symptom of developers not being proficient in advanced source control features, but on the other hand source control does not really go all the way in solving the problem that commenting out is intended for. I've occasionally seen requirements flip-flop in such a way that code that would have been deleted suddenly is needed again. If the code is really deleted, there doesn't seem to be a clear procedure for locating exactly where to find that deleted code in source control.

Is there a better way out there to save developer work that may be needed again besides the tried-and-true "commenting out"?

Improve this question
5
  • 6
    possible duplicate of Can commented-out code be valuable documentation? See also: Is commented out code really always bad? I for one prefer to track code changes via an issue tracker integrated with VCS, which works better than commenting out
    – gnat
    Oct 7, 2013 at 21:52
  • conditional compilation might work
    – ratchet freak
    Oct 7, 2013 at 21:56
  • @ratchetfreak: ugh that's even worse, at least commented out code is immediately obvious its not being used.
    – whatsisname
    Oct 8, 2013 at 3:05
  • 3
    @gnat: Not a dup - OP is asking for an alternative to commenting out code, not if its good, bad or ugly..... Maybe expand you comment into an answer.
    – mattnz
    Oct 8, 2013 at 3:19
  • 2
    @mattnz While not duplicates, those questions do give good discussions of the issues.
    – andy256
    Oct 8, 2013 at 5:32

8 Answers 8

Reset to default
21

We're pretty rigorous about this, and the guideline is that commented-out code shouldn't be checked in. The rationale behind this is that you can never really know whether the engineer forgot to uncomment it before checking it in. Even with a comment explaining why the code is commented out, there's still an element of doubt. If you need it back, you can get it back from source control.

In my opinion, the danger of changing requirements is less than the danger (and probable uncertainty) of seeing a block of checked in commented out code (particularly at some point far in the future) and wondering whether it should be there or not.

At the end of the day, even with specifications, user stories, Jira tickets and all that jazz, "the code is the documentation" - it's the only definitive reference as to what something actually does.

Of course it's just a guideline; I'm sure you're going to get some really great answers to this -- I'm looking forward to reading them!

Succinctly: don't check in the commented code, instead leave yourself a note in your issue tracker. Write in the revision number and location of the alternate implementation. The code remains clean, and you help your future self. If/when the requirements change, the implementation ticket will (should) get reopened, and you can use your breadcrumb to find the previous version. Some issue trackers automatically track the change in source control directly on the ticket - that's another aid to you.

Improve this answer
6
  • 2
    Closing as a dup isn't a bad thing at all (and its just suggestions of them at this time). A well written duplicate question to one that people aren't aware of can help the overall content and structure of the site to make it easier for people to find them later.
    – user40980
    Oct 7, 2013 at 21:55
  • 6
    Just remembered - I've also left "notes to myself" in Jira comments along the lines of "If such-and-such functionality is required in the future, it's in SomeFile.java, SVN rev #XYZ"... as a sort of breadcrumb trail for myself.
    – Faelkle
    Oct 7, 2013 at 21:55
  • If we're ever so uncertain about changing requirements that we want to keep it around, it's easier to keep either a diff or a local copy of the whole file, neither of which is ever checked in to source control. A month down the line, "Oh, what's this not-checked-in file? Hmm... Ah, an old version. Don't need that anymore! rm", and the checked-in code never got cluttered up.
    – Izkata
    Oct 8, 2013 at 2:07
  • 3
    -1 Does not answer the question.
    – mattnz
    Oct 8, 2013 at 3:18
  • @mattnz I'll add my breadcrumb comment up into the answer.
    – Faelkle
    Oct 8, 2013 at 5:10
6

The answer is to delete it. The question is then what to do with it after deleting so that one can find it again. This depends in part on the VCS.

Just deleting has the problem that you can't easily find something with blame (git and svn both have a feature to show where something was added - and the Coding Horror blog post - Who Wrote This Crap?).

However, if you put a comment in its place it does show up in the annotations and in the current code.

One could (for example), replace the block of code with

/* code removed here that does frob()
 * no longer needed because of XYZ requirement
 * see f7f6023 for removed code
 */

You could probably simplify that to one line or something so that it isn't quite the eyesore that I have it. The idea is to leave a 'folded corner of the page' in the code so that someone can find it again. Granted, don't do this for every block of code you take out. If you're just taking out a rather boring line, take it out and be done with it. No need to clutter the mind when reading the source (that is the spot for the commit message)

I'll point out that git has a slight advantage here of being able to specify the checksum of the parent more easily than other VCS can specify the location of the parent (it might be in a branch, it might not be)

If you are on Github or a similar in house repository, one could instead extract it out to a gist and reference that in the comment.

If you aren't in as enlightened of an a VCS system, one can move the idea of a gist into its own 'snippets' directory that one references for sizable chunks of functionality that you want to keep around.

The extraction method of the gist and the snippets has the advantage that one can search easily within this code. You shouldn't be using it only for deleted code, but rather the templates for how to do something. That something was deleted at some point in time isn't a reason not to save that snippit.

To this end, I'd suggest also looking that the question Best practices for sharing tiny snippets of code across projects for ideas on ways to best do that.

Another approach is to save the patch you did that removed the code in question in the issue tracking system itself. Saving the patch for the code remove also saves the code that was removed. Searching the issue tracking system then will also search removed code.

Improve this answer
4
  • 1
    Have you every worked on 30 year old code full of useless comments "/*code removed here for xyz*/ - its as bad as commented out code. The only thing it is useful for is increasing the comment to code ratio metric.....
    – mattnz
    Oct 8, 2013 at 3:23
  • 1
    I've worked with multiple forms - commented out, revisions commented everywhere (ugh, 20 line long header comments of revisions) deleted and extracted. The problem being asked is how to save what is removed when it has some value. The cure might be worse than the problem, but that is a solution to the problem as asked. And frankly, I don't find extractions that bad. A later review and one decides that that spot of code doesn't have value and it's gone.
    – user40980
    Oct 8, 2013 at 3:39
  • git's checksum isn't as useful as svn's revision number - same difference all in all, but the svn revnum is way easier to read :)
    – gbjbaanb
    Oct 10, 2013 at 11:37
  • @gbjbaanb they each have their advantages and disadvantages. That said, the full checksum isn't needed - often the first 4 bytes is used - fb04db7c
    – user40980
    Oct 10, 2013 at 14:44
5

Assuming that source control is in place, creating a branch (or shelf/stash) for the commented out code would probably be easier to maintain than having to go through a bunch of files and un/re-comment a whole bunch of code. It also serves the secondary purpose of logically separating that code from the rest of the code base which is more stable.

Improve this answer
2
  • 2
    I think you might end up in branch hell when requirements change very often. A branch is very difficult to maintain because it is out of focus of everyone and it gets outdated very fast.
    – SpaceTrucker
    Oct 8, 2013 at 6:35
  • @SpaceTrucker I think in the case where the OP described a requirements "flip-flop", creating a branch for the flip (or flop) code wouldn't be unmanageable. It's a reasonable means of preserving the alternative methodology, especially if the code itself isn't changing, just being swapped back in. OTOH, creating branches for minor code cleanups, refactorings, etc. is certainly not an ideal solution.
    – Dan1701
    Oct 8, 2013 at 16:48
2

Some good answers here already, but I would like to add one point.

I've occasionally seen requirements flip-flop in such a way that code that would have been deleted suddenly is needed again.

That is the point you should work on: better identifying the requirements and the reason why they change. If they change because the first analysis was not proper, or your client needed some prototypes until he understands what he really wants, the chances are high that they will stabilize after some iterations. Then it gets obvious that you can delete the older code and keep only the code which does what it should.

If, however, your requirements change because its part of the business (for example, requirement A is for January, March and June, and B for February, April and July), then it might be probably wrong to support only just one implementation - in such case, your application may better support those different requirements at once, either by supporting them in a generalized way, or by allowing some customizing or configuration. In such a case, all your code stays in the application, but not in an outcommented part, but in an active part.

The latter can also be a solution if you have to deal with changing requirements in a prototyping phase as long as the knowledge about the real requirements has not stabilized. In this case, a feature toggle may be a feasible solution.

Improve this answer
1
  • 1
    +1. Right, if there is actually more than one requirement then something more complex is required (and there are a large number of patterns to support that in elegant ways). But I do agree, the actual underlying problem isn't with source code, it's with the process.
    – Faelkle
    Oct 8, 2013 at 8:57
1

I would like to extend the idea of the now deleted answer of @MathewFoscarini.

Create an abstraction (an interface in the simplest case) around the code in question. Make the code in question an implementation of it and create a new implementation that matches with the new requirements. If needed move the old implementation to it's own module, so that it won't be packaged for production.

To summarize: you are running into a violation of the open closed principle, because you are often changing the implementation details of a class. To solve this you need to introduce a common interface and configure your application to use the correct implementation.

Improve this answer
3
  • The trouble is code changes are not always localisable to some abstraction, they may be crosscutting. You might be able to use aspect-oriented programming there, but personally I wouldn't recommended it.
    – Robin Green
    Oct 10, 2013 at 9:56
  • @RobinGreen The abstraction I'm talking about may also be a configuration profile for each different requirement set.
    – SpaceTrucker
    Oct 10, 2013 at 10:00
  • the answer you refer to is deleted and only 10K rep readers can see it; would you mind editing your post to account for that?
    – gnat
    Oct 10, 2013 at 22:22
0

My pragmatic approach is to say its fine to comment out code and check it in. This has the benefit of being able to see the previous code immediately present to anyone looking (as we know, bugs are often introduced and found straight away, so if you leave some old code in there, anyone looking for a bug will be much more likely to spot where the offending change might be).

However, I also have the thought that any commented out code can be deleted when you next check it out for editing. So if you get some code and see commented out code, then its now past its usefulness and can safely be deleted. Commented out code should then only live for 1 checkin. (of course, you don't checkin code that only has its commented code removed, only if you change the area around it as you should never see that commented-out code if you never have to look at the code again).

Obviously its a very rough guideline, and I only comment out code I think is a bit hairy anyway.

Improve this answer
0

If you know that the code may be used again, I would refactor it out as an unused method and document the fact that it is available for possible future use.

If it must or should be removed, then I would consider using some form of tagging or a standard comment like REMOVED:, which is searchable later.

Improve this answer
0

I think don't think you should operate under the premise that; We may need it. If we don't have it and need it there will be this disproportionate amount of time to recreate the solution. This solution is so great no one will ever think of it again.

If possible, write a unit test that passes based on this line(s) of code being excluded. Something about the test should tell you why it is no longer a good idea to have this code in there.

Now, you have it documented in: source control, testing and anything other documentation, project or request tracking.

The goal isn't to find the most convenient way to add back, but to rely on and review all the resources about a particular area of the application before making any changes. Just using memory alone or unnecessary prompts in your code is not enough. Who knows if something else changed that makes the saved code useless.

YAGNI

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.