Too Many Comments?

[Pushkar Choudhary]

I had recently applied at a relatively small but well known MNC who are into IT consulting, majorly on Agile methodologies.

As a part of the interview process, the candidates have to solve one out of two coding problems given by the company. I was also given two such coding problems and solved one of them. After a week from the day I submitted the code, I received a negative reply from them. They also provided their feedback for the code.

The feedback had 2 comments, one of which was "Too Many Comments. More comments than code." I was very surprised by this comment. I always thought the code should be well documented with comments, which is what I had done. I had provided about 2-3 lines class level comments for the main class and 1-2 lines for the other classes. And I had provided 1-2 lines comments for each method. I'm not sure if this would classify as "Too many comments" or "More comments than code".

So, the question is, is it bad coding practice to provide comments for every class, method, etc.? I would really appreciate anyone's input, since I have always practiced coding with comments, assuming it contributes to a better code.

-Pushkar

[Sunil Tiwari]

Thanks for the "master" coders who "fail" to write comments in legacy applications!
Well writing comments in code won't make it any better but more readable, hence helping programmers/support engineers "inherit" it with ease.
I support writing comments!

[Deepak Bala]

It depends on what 'too many' implies for a developer. Here is what I usually do

* A class needs a few words describing what it does.

* Each public method need about a line of comment describing what it does.

* I refrain from providing comments inside methods. When I do provide them its usually not more than a line and it denotes how the control flows. Something like

  
if(something)
{
    // Relation with A
    ... 20 LOC ...
    // Relation with B
    ... 20 LOC ...
}
  

I try to provide very descriptive method names and variable names instead of comments. So there are times when my variable names are longer than necessary. My definition of long is 20-30 characters. Public static constants are usually as long as that, if not longer.

The reason I dislike too many comments is that developers never update them. Here is a story...

I used to work on a code base with a lot of comments. It was unstable. In a meeting, the team was discussing with an architect about EJBs and performance and HTTP calls etc etc related to the code called by a method. I didnt understand why they were doing this since the code never makes an EJB call. But I decided to keep quiet thinking someone in the meeting knew something that I didnt. After the meeting was over I asked 'So why are you guys discussing EJBs ?'. One of the devs told me 'Well look, it says here on top of the method that it makes an EJB call to EJBX.' It turned out that EJBX did not exist and no EJB calls were made from the code.

The other reason I do not like lots of comments is because it breaks the way I read code. I scan the code and misplaced / misused comments distract me.

No comments in code is as bad as too many comments. But how many are necessary is subjective and also depends on the complexity of the code in question. So while there is no general answer to the question you raised, the views represented above might help you find out why your reviewer thought there were too many comments. Or perhaps your reviewer needs to redefine what 'too many' is.

So what was the other comment ?

[Edit]

Corrected spelling

[Sandeep Awasthi]

In IMO, when you write a code, you have to see how easy/difficult would be for new person to understand it. It depends how complex/easy the project/module/code is and your comments should be according to that. Focus should be when a new person comes, he/she should be able to understand it, but it should not be too much to maintain. Is too many comments is problem? IMO your code is should not look replacement for other documentation. Many things can be understood with naming conventions as Deepak said. But comments should be included where they are needed but they are not replacement for other documentation.

[Kj Reddy]

I will try to provide as much comments as possible for all public methods/variables so that its easier to reuse the code for other developers.

I will try to avoid as much comments as possible in inline with code. Preferably you variable & method name conventions should self explanatory to understand the code. I will provide inline comments in scenarios like: reasons explain when some code is odd, to explain complex logic, when done some hard coding etc.,

[Pushkar Choudhary]

Ok. Maybe I didn't mention clearly. Let me try once more.

My code did not have ANY comments inside a method. When I talking about comments, I was referring to the class-level and method-level comments which describe what that class or method does. Something like this:

	/**
	 * Sets the name of the log file.
	 * 
	 * @param path
	 *            the path for the log file
	 * @return file name containing the path
	 */
	private String setLogFile(String path) {
         ......
         ......
    }

Apart from this, there were no comments inside any methods.

[Sandeep Awasthi]

I personally would not like plain get and set methods to have comment because they are standard java bean conventions and should be understood.

[edit] If it is private method contains some logic and not java bean public get/set method, please ignore my above comments.

[Jaikiran Pai]

Pushkar Choudhary wrote:

My code did not have ANY comments inside a method. When I talking about comments, I was referring to the class-level and method-level comments which describe what that class or method does. Something like this:

	/**
	 * Sets the name of the log file.
	 * 
	 * @param path
	 *            the path for the log file
	 * @return file name containing the path
	 */
	private String setLogFile(String path) {
         ......
         ......
    }

Apart from this, there were no comments inside any methods.

That certainly does not fall under "Too many comments" category. Some might debate that getter/setter do not merit a javadoc. But I usually, make it a point to even have javadocs for such methods. That cannot be considered a bad practice.

They also provided their feedback for the code.

The feedback had 2 comments, one of which was "Too Many Comments. More comments than code."

If they are open to discussions, maybe ask them why they consider this a bad practice and how does it qualify as "too many comments". I guess they are just concentrating on the number of lines in that getter/setter method and then comparing it with the number of lines of javadoc comment. The "more comments than code" itself is a debatable topic. I personally find it useful to add as much comments (javadoc or code comments) as possible, so that some one else doesn't have to fight hard to understand why a particular piece of code exists.

[Kj Reddy]

Pushkar Choudhary wrote:Ok. Maybe I didn't mention clearly. Let me try once more.

My code did not have ANY comments inside a method. When I talking about comments, I was referring to the class-level and method-level comments which describe what that class or method does. Something like this:

	/**
	 * Sets the name of the log file.
	 * 
	 * @param path
	 *            the path for the log file
	 * @return file name containing the path
	 */
	private String setLogFile(String path) {
         ......
         ......
    }

Apart from this, there were no comments inside any methods.

Sorry. In my view the JavaDoc comments do not fall under "Too many comments" including JavaDocs for getter/setter methods. JavaDocs are quite useful to understand what the public method/variable will do without looking into the source code.

Ask them if they can provide more details on the feed back by providing them your opinion.

[Deepak Bala]

Ask them if they can provide more details on the feed back by providing them your opinion.

Do note that they may not respond back. I agree with Reddy that you should squeeze as much info as possible. But you may not get it.

  
 /** 
      * Sets the name of the log file. 
      *  
      * @param path 
      *            the path for the log file 
      * @return file name containing the path 
      */  
     private String setLogFile(String path) {  
          ......  
          ......  
     }  
  

That is fine except for 2 things

1. This is a private method. Javadocs for public methods usually derive more benefit, but this is not true all the time.
2. One could argue that setter methods do not need javadocs.

These are just observations. I am not arguing to or for them. But calling this 'too many comments' is not justified. Too many comments is something like this

  
 /** 
      * This method sets the log file.
      * My manager asked me to write this javadoc, 
      * but you know what, I am just putting this stuff here,
      * so checkstyle will not catch it and raise a flag; I
      * know no one will read my comments so I guess
      * I can write anything that I want here. In fact  
      * checkstyle now think my method is very very very well documented. I hope
      * I get a star for this. 
      *  - Signed the programmer.
      */  
     private String setLogFile(String path) {  
          ......  
          ......  
     }  
  

[Sandeep Awasthi]

Well this is not too many comments. This is something else

[Vikas Kapoor]

JavaRanch Granny wrote:Debug only code - comments can lie.

When functionality changes, you are bound to change the comments as well. And If too many comments too much burden.

[Campbell Ritchie]

If your functionality changes are requires a change in the documentation comments, then you are altering the public interface of your class and probably "breaking" other code

[Mike Isano]

There is some thought that code should be self describing, and not need a comment.

   

//this needs a comment explaining what greater than 70 means
if(score >= 70)
{
  //blah
}


//this does not need a comment. Explaining that 70 is a passing score here would be redundant.
boolean isPassingScore = score >= 70; 
if(isPassingScore)
{
 //blah
}
 

[David Newton]

Pushkar Choudhary wrote:Ok. Maybe I didn't mention clearly. Let me try once more.

My code did not have ANY comments inside a method. When I talking about comments, I was referring to the class-level and method-level comments which describe what that class or method does. Something like this:

	/**
	 * Sets the name of the log file.
	 * 
	 * @param path
	 *            the path for the log file
	 * @return file name containing the path
	 */
	private String setLogFile(String path) {
         ......
         ......
    }

Apart from this, there were no comments inside any methods.

I actually have a few issues with this code/comment snippet.

1) It's *not* a simple setter, but:
    a) doesn't explain why not,
    b) violates JavaBean naming conventions meaning it may not even be *usable* as a setter in some circumstances,
    c) doesn't follow fluent interface naming convention ("logfile(path)")
2) It's not actually named what it does--I'd expect it to take a File, not a String. Why not name it setLogFilePath (or setLogfilePath, or...)
    a) and the method comment says it sets the name, but the parameter is called "path": which is it? Is it relative? Absolute? Depends on...?
3) Javadoc will automatically extract parameters. There's no reason to add a @param tag if it provides no additional information not provided by naming, so:
    a) private String setLogFilePath(String path) or (String thePath) is plenty of info.

Since it *does* violate JavaBean conventions, I have less of an issue with the @return tag, but that could just as easily be written as part of the method comment:

/** Sets log file path, and returns same.  */
// or
/** Sets and returns log file path. */

So my issues aren't precisely that there are "too many comments", but rather they're not consistent with names, not consistent with convention (with no reason provided as to why), and with better naming, they become redundant at best, and just another potential maintenance failure at worst.

The comments themselves might not be enough for me to reject the code, but I *would* have to think twice about the naming conventions, depending on the larger context--I can't *really* say anything useful without having more context.

This is a particularly interesting question to me because my current interviewing process also includes a take-home coding exercise and I need to evaluate what's given back to me. I expect it will be interesting, if time-consuming. And I *definitely* have strong preferences about code, code style, commenting style, etc. which probably don't match with a lot of other peoples' ideas of what's "right".

[David Newton]

Mike Isano wrote:

boolean isPassingScore = score >= 70; 
if(isPassingScore) { ... }

Even better, for me, would be

if (isPassingScore(score)) { ... }

because then isPassingScore is testable in isolation, *and* eliminates the need for a comment.

(And depending on context, I might use a scoring implementation for that as well, or allow a dynamic formula... 'cuz that's just how I am.)