Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I wrote the following code:

if (boutique == null) {
    boutique = new Boutique();

    boutique.setSite(site);
    boutique.setUrlLogo(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getLogo());
    boutique.setUrlBoutique(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getUrl());
    boutique.setNom(fluxBoutique.getNom());
    boutique.setSelected(false);
    boutique.setIdWebSC(fluxBoutique.getId());
    boutique.setDateModification(new Date());

    boutiqueDao.persist(boutique);
} else {
    boutique.setSite(site);
    boutique.setUrlLogo(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getLogo());
    boutique.setUrlBoutique(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getUrl());
    boutique.setNom(fluxBoutique.getNom());
    //boutique.setSelected(false);
    boutique.setIdWebSC(fluxBoutique.getId());
    boutique.setDateModification(new Date());

    boutiqueDao.merge(boutique);
}

There is a commented-out line here. But I think it makes the code clearer, by making obvious what the difference is between if and else. The difference is even more noticeable with color highlighting .

Can commenting out code like this ever be a good idea?

share|improve this question

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

13 Answers 13

up vote 74 down vote accepted

Most of the answers focus on how to refactor this one specific case, but let me offer a general answer to why commented out code is usually bad:

First, commented out code isn't compiled. This is obvious, but it means that:

  1. The code might not even work.

  2. When the comment's dependencies change it will not obviously break.

Commented code is very much "dead code". The longer it sits there, the more it rots and provides less and less value to the next developer.

Second, the purpose is unclear. You really need a longer comment that provides context for why there are randomly commented lines. When I see just a commented line of code, I have to research how it got there just to understand why it got there. Who wrote it? What commit? What was the commit message/context? Etcetera.

Consider alternatives:

  • If the goal is the provide examples of using a function/api, then provide a unit test. Unit tests are real code, and will break when they are no longer correct.
  • If the purpose is to preserve a previous version of the code, use source control. I'd much rather checkout a previous version then toggle comments throughout the codebase to "revert" a change.
  • If the purpose is to maintain an alternate version of the same code, use source control (again). That is what branches are for, after all.
  • If the purpose is to clarify structure, consider how you can restructure the code to make it more obvious. Most of the other answers are good examples of how you might do this.
share|improve this answer
3  
I think you are missing one important reason: Documentation: If the purpose is to document alternate design options an explanation of the alternative and especially the reason why it has been discarded should be provided instead of the original code. –  Sarien Apr 5 '13 at 8:22
8  
Design options are better explained in a human language than in a programming language. –  mehaase Apr 5 at 17:50

This is an absolutely horrifying idea. It does not make clear what the intent is. Did the developer comment out the line by mistake? To test something? What's going on?!

Aside from the fact that I see 6 lines that are absolutely equal in both cases. Rather, you should prevent this code duplication. Then it will be clearer that in one case you additionally call setSelected.

share|improve this answer
7  
Agreed. I'd assume seeing that that the commented-out line is old behaviour that has been removed. If a comment is needed, it should be in natural language, not code. –  Jules Mar 12 '13 at 18:29
3  
I agree entirely! I have recently spent hours on end trying to understand and clean up some applications that I've inherited that are almost completely unreadable because of this practice. It also includes code that has been disconnected from all other code but not removed! I believe that this a main purpose behind version control systems. It has comments as well as the changes that go with them. In the end, I have had at least 2 weeks of work added to my plate in great part because of this practice. –  bsara Mar 15 '13 at 3:06

The biggest problem with this code is that you duplicated those 6 lines. Once you eliminate that duplication, that comment is useless.

If you create a boutiqueDao.mergeOrPersist method you can rewrite this as:

if (boutique == null) {
    boutique = new Boutique();
    boutique.setSelected(false);
}

boutique.setSite(site);
boutique.setUrlLogo(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getLogo());
boutique.setUrlBoutique(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getUrl());
boutique.setNom(fluxBoutique.getNom());
boutique.setIdWebSC(fluxBoutique.getId());
boutique.setDateModification(new Date());

boutiqueDao.mergeOrPersist(boutique);

Code that either creates or updates a certain object is common, so you should solve it once, for example by creating a mergeOrPersist method. You certainly should not duplicate all the assignment code for those two cases.

Many ORMs have built in support for this in some way. For example they might create a new row if the id is zero, and update an existing row if the id is not zero. The exact form depends on the ORM in question, and since I'm not familiar with the technology you're using, I can't help you with that.


If you don't want to create a mergeOrPersist method, you should eliminate the duplication in some other way, for example by introducing a isNewBoutique flag. That may not be pretty, but it's still much better than duplicating the whole assignment logic.

bool isNewBoutique = boutique == null;
if (isNewBoutique) {
    boutique = new Boutique();
    boutique.setSelected(false);
}

boutique.setSite(site);
boutique.setUrlLogo(CmsProperties.URL_FLUX_BOUTIQUE + fluxBoutique.getLogo());
boutique.setUrlBoutique(CmsProperties.URL_FLUX_BOUTIQUE + fluxBoutique.getUrl());
boutique.setNom(fluxBoutique.getNom());
boutique.setIdWebSC(fluxBoutique.getId());
boutique.setDateModification(new Date());

if (isNewBoutique)
    boutiqueDao.persist(boutique);
else
    boutiqueDao.merge(boutique);
share|improve this answer

Commented-out code is not used by the application, so it needs to be accompanied by further comments stating why it is not being used. But within that context, there are situations where commented-out code can be useful.

What comes to my mind is a case where you solve a problem using a common and appealing approach, but then it turns out that the requirements of your actual problem are slightly different from that problem. Especially if your requirements turn out to require considerably more code, the temptation for maintainers to "optimize" the code using the old approach will likely be strong, but doing that will only bring the bug back. Keeping the "wrong" implementation around in the comments will help to dispel this, because you can use it to illustrate exactly why that approach is wrong in this situation.

This is not a situation that I can imagine occurring very often. Usually, it should be sufficient to explain things without including a sample "wrong" implementation. But I can imagine a case where that's not enough, so since the question is about whether it can be useful, yes, it can. Just not most of the time.

share|improve this answer
1  
Sorry, but I fail to see any value of comment-out code. Code that is commented out is not used, hence it has no place in production code. –  Vladimir Kocjancic Apr 5 at 19:08
1  
Please define "used". –  JensG Apr 5 at 19:30

I would just like to add to CodesInChaos's answer, by pointing out that you can refactor it further into small methods. Sharing common functionality by composition avoids the conditionals:

function fill(boutique) {    
  boutique.setSite(site);
  boutique.setUrlLogo(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getLogo());
  boutique.setUrlBoutique(CmsProperties.URL_FLUX_BOUTIQUE+fluxBoutique.getUrl());
  boutique.setNom(fluxBoutique.getNom());
  boutique.setIdWebSC(fluxBoutique.getId());
  boutique.setDateModification(new Date());
}    

function create() {
  boutique = new Boutique();      
  fill(boutique);
  boutique.setSelected(false);
  return boutiqueDao.persist(boutique);
}

function update(boutique) {
  fill(boutiquie);
  return boutiquieDao.merge(boutique); 
}

function createOrUpdate(boutique) {
  if (boutique == null) {
    return create();
  }
  return update(boutique);  
}
share|improve this answer
5  
I think that's the cleanest suggestion here. –  Alexis Dufrenoy Mar 11 '13 at 15:07
    
+1, and I would also add that the more you avoid passing around null objects, the better (I find this solution is a good example). –  Nadir Sampaoli Mar 12 '13 at 9:00
    
I would pass boutiqueDao as inputs to create and update. –  Happy Green Kid Naps Mar 12 '13 at 17:56
    
How can this work tho? How can you know when to call create and when to call update? The original code looks at boutique and knows if it needs to update or create. This just does nothing until you call create or update... –  Lyrion Mar 13 '13 at 14:36
    
Lyrion: Trivial, I'll add that code aswell for clarity. –  Alexander Torstling Mar 13 '13 at 15:51

This doesn't look good buddy.

Commented code is...just not code. Code is for implementation of logic. Making a code more readable in itself is an art. As @CodesInChaos have suggested already that repetitive lines of code are not very good implementation of logic.

Do you really think that one true programmer will prefer readability over logical implementation. (by the way we have comments and 'complements' to put in our logical representation).

As far as I am concerned one should write a code for compiler and that's good - if 'it' understands that code. For human readability Comments are good, for the developers (in a long run), for people reusing that code (e.g. testers).

Otherwise you can try something more flexible here, something like

boutique.setSite(site) can be replaced with

setsiteof.boutique(site). There are different aspects and perspective of OOP through which you can increase readability.

While this code seems to be very appealing at first and one can think that he have found a way for human readability while compiler also does its job perfectly, and we all start following this practice it will lead to a fuzzy file which will become less readable in time and more complex as it will expand its way down.

share|improve this answer
12  
"As far as I am concerned one should write a code for compiler" Oh please, don't. That's how you end up with monstrocities that look like they could be taken straight from the Obfuscated C Contest and the likes. Computers are binary, while humans use fuzzy logic (this goes double for pet owners, by the way). Computer time is next to free today (basically just electricity usage) whereas programmer time is comparatively very expensive. Write code for humans, and the compiler will understand it. Write code for the compiler without regard for humans, and you won't make many friends on the team. –  Michael Kjörling Mar 12 '13 at 15:24
    
"write code for a compiler" - Actually you don't. The person you should have in mind is the one who's handed over the task to maintain your code. –  JensG Apr 5 at 19:29

While this is clearly not a good case for commented out code there is a situation that I think warrants it:

// The following code is obvious but does not work because of <x>
// <offending code>
<uglier answer that actually does work>

It's a warning to whoever sees it later that the obvious improvement isn't.

Edit: I'm talking about something small. If it's big you explain instead.

share|improve this answer
4  
What's wrong with // the following part done like it is because of X? Explain why you did something the way you did, not why you did not to it in some particular way. In your particular example, it eliminates the need for potentially a large block of commented-out code entirely. (I didn't downvote, but can certainly see why this would get downvoted.) –  Michael Kjörling Mar 12 '13 at 15:27
6  
Michael, because it makes it clear to other coders (and yourself days/weeks/months later) that yes, you did try that cleaner/more clever approach, but no, it didn't work because of X, so they shouldn't bother trying it again. I think this is a completely legitimate approach and upvoted this sadly buried answer. –  Garrett Albright Mar 13 '13 at 6:01
    
@GarrettAlbright: Thank you, I'm glad to see someone gets it. –  Loren Pechtel Mar 13 '13 at 19:31
    
@LorenPechtel: Not only that, I was about writing more or less exactly the same. There are situations where it is very, very useful to quickly know which "obvious" solutions have been already tried without success and why they don't work. –  JensG Apr 5 at 19:24

The only time I've seen commented out code that was useful was in config files, where the code for every option is provided, with many options commented out. This makes it easy to switch features on and off by just removing comment markers.

## import this list of modules
# config.imports = ['os', 'sys']
share|improve this answer

No, commented code gets stale, and is soon worse than worthless, it is often harmful, as it cements some aspect of implementation, along with all of the current assumptions.

Comments should include interface details and intended function; "intended function": can include, first we try this, then we try that, then we fail this way.

The programmers that I have seen try to leave things in comments are just in love with what they have written, don't want to lose it, even if it is not adding anything to the finished product.

share|improve this answer

In this specific example, I find the commented-out code very ambiguous, largely for the reasons outlined in Dibkke's answer. Others have suggested ways that you could refactor the code to avoid even the temptation to do this, though, if that's not possible for some reason (e.g., the lines are similar, but not quite close enough), I would appreciate a comment like:

// No need to unselect this boutique, because [WHATEVER]

However, I think there are some situations where leaving (or even adding commented out) code is not reprehensible. When using something like MATLAB or NumPY, one can often write equivalent code that either 1) iterates over an array, processing one element at a time or 2) operates the entire array at once. In some cases, the latter is much faster, but also a lot harder to read. If I replace some code with its vectorized equivalent, I embed the original code in a nearby comment, like this:

%% The vectorized code below does this:

% for ii in 1:N
%    for jj in 1:N
%      etc.

% but the matrix version runs ~15x faster on typical input (MK, 03/10/2013)

Obviously, one needs to take care that the two versions actually do the same thing and that the comment either stays in sync with the actual code or is removed if the code changes. Obviously, the usual caveats about premature optimization also apply...

share|improve this answer
    
"Obviously, one needs to take care that the two versions actually do the same thing and that the comment either stays in sync with..." - right there you explained why this is not a good idea. –  sleske Aug 27 at 14:38
    
Well, that's a problem with all comments, right? Some vectorized code is sufficiently opaque that comments are worthwhile, and having an "unrolled" version can be handy for debugging. –  Matt Krause Aug 27 at 17:07
    
True. Still, I'd try to keep the comment as brief as possible, not use full source code. Anyway, if you have an example, asking how to best make it readable would be a good question (here or on codereview.s.e). –  sleske Aug 28 at 5:25

It can be in very rare cases, but not as you've done it. The other answers have pretty well hashed out the reasons for that.

One of the rare cases is a template RPM spec we use in my shop as a starting point for all new packages, largely to make sure nothing important is left out. Most, but not all of our packages have a tarball containing sources which has a standard name and is specified with a tag:

Name:           foomatic
Version:        3.14
 ...
Source0:        %{name}-%{version}.tar.gz

For packages without sources, we comment out the tag and put another comment above it to maintain the standard format and indicate that someone has stopped and thought about the problem as part of the development process:

Name:           barmatic
Version:        2.71
 ...
# This package has no sources.
# Source0:        %{name}-%{version}.tar.gz

You don't add code you know isn't going to be used because, as others have covered, it could be mistaken for something that belongs there. It can. however, be useful to add a comment explaining why code one might expect to be there is missing:

if ( condition ) {
  foo();
  // Under most other circumstances, we would do a bar() here, but
  // we can't because the quux isn't activated yet.  We might call
  // bletch() later to rectify the situation.
  baz();
}
share|improve this answer
5  
arguably that comment isn't commented out code though. –  jk. Mar 11 '13 at 15:48
1  
@jk: You are arguably correct. –  Blrfl Mar 11 '13 at 15:53

Generally speaking, code is only self-documenting to the person who wrote the code. If documentation is required, write documentation. It is unacceptable to expect a developer new to a source-code base will sit down read thousands of lines of code to attempt to figure out from a high-level what is happening.

In this case, the purpose of the commented-out line-of-code is to show the difference between two instances of duplicate code. Instead of attempting to subtely documenting the difference with a comment, rewrite the code so it makes sense. Then, if you still feel it is necessary to comment on the code, write an appropriate comment.

share|improve this answer
2  
This rings quite true. A lot of people (myself included) think their code is so awesome that it doesn't need documentation. However, everyone else in the world finds it gobbledygook unless it's thoroughly documented and commented. –  Ryan Amos Mar 12 '13 at 3:04
    
"code is only self-documenting to the person who wrote the code" - Please pick a piece of complex, uncommented code you wrote a year ago and try tro understand it in a limited amount of time. You can't? Ooops. –  JensG Apr 5 at 19:26

No, it's a terrible idea. Based on that piece of code the following thoughts come up to my mind:

  • This line is commented out because the developer was debugging it and forgot restore the line to its former state
  • This line is commented out because it once was part of the business logic, but it is no longer the case
  • This line is commented out because it caused performance problems on production and the developer wanted to see what the impact was on a production system

After seeing thousands of lines of commented out code, I'm now doing the only sensible thing when I see it: I immediately remove it.

There is no sensible reason to check in commented out code into a repository.

Also, your code uses a lot of duplication. I suggest you optimize that away for human readability as soon as possible.

share|improve this answer
1  
However I get rid of the duplicated code, it can hardly be seen as an optimization, I think. –  Alexis Dufrenoy Mar 11 '13 at 9:49
20  
it is an optimization for human readability –  jk. Mar 11 '13 at 10:23
10  
@Traroth you can optimize for speed, memory use, power consumption or any other metric so I don't see that you can't optimize for readability (though as a metric it is a bit woollier) –  jk. Mar 11 '13 at 10:44
2  
Indeed, I meant human readability. Small hint here: your most important liability in programming is your code. So, less is truly more here. –  Dibbeke Mar 11 '13 at 13:23
3  
Software as a liability is also treated at c2.com/cgi/wiki?SoftwareAsLiability From there: "Producing more code is not always a gain. Code is expensive to test and maintain, so if the same work can be done with less code that is a plus. Don't comment out dead code, just delete it. Commented out code goes stale and useless very fast, so you may as well delete it sooner rather than later, to loose the clutter. Keep good backups to make it easier." –  ninjalj Mar 11 '13 at 23:53

protected by maple_shaft Mar 11 '13 at 22:33

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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