Can commented-out code be valuable documentation?

Question

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, even more with color highlighting .

Do you think it is a good idea?

Answer 1

Most of the answers are focusing 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) it might not even work and 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? etc.).

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

Answer 2

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 = isboutique == 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);

Answer 3

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

Besides that I see 6 lines that are absolutely equal in both cases. You should rather prevent this code duplication, then it'll be clearer that in one case you additionally call setSelected.

Answer 4

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.

Answer 5

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);  
}

Answer 6

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

Answer 7

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.

Answer 8

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.

Answer 9

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']

Answer 10

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();
}

Answer 11

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.

Answer 12

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.

Answer 13

No, this is definitely not a good way to code.

Code of this nature are a product of copy-paste (because you think the code you want to write has a lot of things in common with one you already written before) and always lead to duplicate code and even bugs (which I suppose you intended to avoid as well in the process).

I suggest the following instead:

1. Rename your variables or methods to reflect what your intentions are for using them
2. Consider sitting back and redesigning your application whenever you notice possible common implementations. take advantage of composition (and inheritance with caution)
3. Try to have as short as possible number of lines per function

Gradually you'll find you have much less need for commented code.