views:

465

answers:

24

I'm trying to make my code easily understood by future readers.

I've always had issues with how to word my if statement comments to make it the most understandable.

Maybe it seems trivial, but it's something that's always bothered me

Here's an example:

if ( !$request ) {
    $request = $_SERVER['REQUEST_URI'];
}

Here are some way I can think of commenting it

// If request doesn't exist
if ( !$request ) {
    // Set request to current request_uri
    $request = $_SERVER['REQUEST_URI'];
}

// Check for a request
if ( !$request ) {
    $request = $_SERVER['REQUEST_URI'];
}

// Request doesn't exist
if ( !$request ) {
    // Set request
    $request = $_SERVER['REQUEST_URI'];
}

Not the best example, but the way I see it there are infinite ways to word it.

I've never really worked on a team so I don't have much experience on other people reading my code.

What are your experiences on the best way to word that to make it readable for future coders.

+4  A: 

I don't want to say that it doesn't matter, but it's mostly up to the programmer's preference. Even better is to write the predicates in the if statement to be readable without commenting.

This is easier in some languages than others, though. For example, some languages use semantic negation keywords like not, which can make predicates easier to read. That being said, if the reader in question is a decent-enough programmer, they should be able to handle a general test's logic without having to be hand-fed annotation.

Bottom line: Use your discretion.

Evan Meagher
+9  A: 

For the cases you provide, I wouldn't comment them at all. I only use comments in the body of a method/function when performing something extremely tricky or non-obvious - two things I try to avoid. Just put one short comment at the beginning of the method.

anon
I'll quibble a little on this comment: in PHP (his example looked like PHP code), (!$request) could be equivalent to two things: if(!isset($request)) or if($request==false). Which is to say, an expression evaluating "false" might mean either, and it might be that that difference is important.On the whole, I agree with your comment! I just wanted to point out that in this example, though the functionality might be obvious ("seeing if there was a request"), the comments point out what we're /actually/ testing for!
rascher
+1  A: 

I don't like to read code littered with comments. The code should be easy to understand. I usually only comment large, difficult to understand portions of code, not individual and trivial lines.

Martinho Fernandes
A: 

The example if statements are simple enough to not warrant any comments. Excessive commenting can be dangerous because then you have to maintain not only the code but the comments. For complex if statements, you have the right idea - stick a comment right above the if statement.

A: 

I agree with neil, for that example anyway it's pretty obvious that you are checking if a request doesn't exist. Putting in to many comments can infact make your code less readable imo. (i.e. commenting every line or every other line of code )

A: 

I agree with Evan - write your code to be readable. Assuming you have complicated code that needs commenting, though, I use and prefer the first option, which flows well as readable text.

Michael Petrotta
+8  A: 

In this particular example I wouldn't bother commenting the if statement -- you are repeating what is said in the code.

I might see a case where the test code is complicated:

if (a == 3 && b && c > 2)

but in that case I would first try to extract a method with a meaningful name:

if (markerIsValid(a, b, c))

Only if that was not possible would I then comment the test.

Kathy Van Stone
+1 - extracting a method not only self documents, but makes the logic re-usable and consistent.
g .
+2  A: 

Using your example, I usually prefer the following style:

if ( !$request ) {
    // The request is not yet set, so retrieve it.
    $request = $_SERVER['REQUEST_URI'];
}

I like to place it inside of the if block to make it look better when there is an else or else if.

if ( !$request ) {
    // The request is not yet set, so retrieve it.
    $request = $_SERVER['REQUEST_URI'];
}
else {
    // Comment about doing something else here.
}
Sebastian Celis
Rather than say "If the request is not yet set," I'd just say, "The request is not yet set." That's because we're already inside the "if" block. When we're there, we know the result of the comparison, so state it as a fact there, not a conditional.
Rob Kennedy
Valid point. Updated my post. Thanks for the comment.
Sebastian Celis
+5  A: 

My recommendation is 'don't state the obvious'.

Reading if ( !$request ) says - if there does not exist a request. I don't need a comment for that.

If you have multiple checks (this || (that && this-too)) I would go for creating a method that returns a boolean with the result. Then your method name is your comment, which typically is better than an actual comment.

Thies
A: 

Comments should explain code that is not clear. In most cases, code that is not clear should be rewritten to be clear. In your example, I do not see any need for a comment at all. So may answer to how to word your comments is to focus on making the code more readable so you don't need comments.

When they are necessary, they should not simply be pseudo code, but should provide information the reader does not have immediately available. An example might be a comment at a "new" statement indicating where memory is released.

TMarshall
A: 

I'd focus more on making the code itself as self-documenting as possible, meaning good variable names, function names, etc. Comments are better relegated for functions or blocks of code than individual lines.

no-op
+2  A: 

Read the book Clean Code by Robert Martin.

http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

Comments should be only used to explain concepts that the code can't explain itself.

Jeffrey Hines
A: 

Don't comment the obvious unless your audience are e.g. beginners (1).

lothar
+1  A: 

It's very much up to the programmer. I will provide two tips:

Do not state the obvious

Take the following example:

// If request doesn't exist
if ( !$request ) {

In the above case, the comment simply duplicates the code (I realize that it was an example, but I still want to make the point). Focus comments on clarifying what might not be obvious in the code. On the other hand...

Don't assume that all developers know what you know

Look at your code and imagine that you are a relative beginner. Would you understand it? If not, clarify with comments.

Fredrik Mörk
+3  A: 

When I write code comments, I try to either

  1. Explain a tricky, non-obvious procedure (reg ex is a perfect example), or
  2. Explain why I'm doing what I'm doing.

Check out Code Complete. McConnell has a great chapter on commenting.

I would recommend the book for everyone who writes code.

BryanH
yes, i have this book, i forgot about the section on commenting. thanks!
Galen
+1  A: 

I use this form:

// Full Comment
if (condition) {
    action;
    action;
}

Because when code-folding is enabled, you end up with something that looks like this:

// Full Comment
if (condition) { ...} [+]

Rather than this:

// Half Comment
if (condition) { ...} [+] <---button to click to get the rest of the comment
sal
A: 

If your variables are well named your if statement should be pretty self explanitory.

Also, with the use of some "strategic" comments it should all be plain english.

Check out the definition of "tactical" and "strategic" comments in this doc http://www.doc.ic.ac.uk/lab/cplus/c++.rules/chap4.html#sect3

Greg B
+2  A: 
BlairHippo
A: 

See this similar/identical question: Where to put comments in an IF THEN ELSE construct

Rabarberski
A: 

Name your variables/methods in such a way that a comment would be trivial.

If you have a lot of complex conditionals in your if then extract this conditional to a boolean function/method that explains clearly what it's doing.

If you need to comment your conditionals you're probably on the road to the Arrow Anti-pattern.

Use composed method refactoring to help with this. Make sure your methods/functions are encapsulated at the same level of abstraction. Polymorphism will allow you to get rid of conditionals altogether. It's better because behavior is determined dynamically at runtime. It's one less thing you have to code (and maintain).

cwash
A: 

As said before, maybe your exemple is a bit too simple in order really have a relevant comment on it, but here are a couple of general suggestions :

  • usually is easier to read "positive" conditions rather than negations ( not condition )

  • don't hesitate to extract methods that will have a detailed name that will communicate the intent and avoid thus the need of some of the comments. In your case, say you create :

function getRequest( $req ) {
    if( isset( $req ) ) {
        return $req;
    } else {
        return $_SERVER['REQUEST_URI'];
    }
}

but again, we need a more appropriate exemple, in your case it might be overkill

  • must read :

--- Refactoring by Martin Fowler

--- Writing Solid Code

--- Code Complete

Billy
A: 

I realize it doesn't apply in your specific example, but as a pedant I am compelled to say: whenever possible, don't lead with the negative case. That's harder to read no matter what the comment. In general I would agree the test should be clear without a comment or you might want to extract it to a meaningfully-named method.

Tom
+1  A: 

You should not have to. Just refactor by introducing explaining variables:

 if ( (platform.toUpperCase().indexOf("MAC") > -1) &&
      (browser.toUpperCase().indexOf("IE") > -1) &&
       wasInitialized() && resize > 0 )
 {
   // do something
 }

becomes:

boolean isMacOs     = platform.toUpperCase().indexOf("MAC") > -1;
boolean isIEBrowser = browser.toUpperCase().indexOf("IE")  > -1;
boolean wasResized  = resize > 0;

if (isMacOs && isIEBrowser && wasInitialized() && wasResized)
{
 // do something
}
aleemb
+1  A: 

I second the reference to Code Complete: an excellent book that should be required reading for programmers.

There are some things I would keep in mind:

  1. Don't repeat the code: explain what it is doing in general terms.

  2. Never assume something is obvious; clarify and explain the code.

  3. Use functions to clarify your meaning as appropriate: make the name relate to what is being done.

  4. Never assume that the code is self-explanatory - even if it should be. Most importantly, never assume that a particular code "idiom" will be read by someone who understands it: explain what the code is doing.

As someone said, don't lead with the negative if you can: I had a programming class where a negative IF was not allowed under any circumstances. If really stuck, you can use your native language (such as English) to your advantage. Instead of:

if (!eof(f))

One can say:

if (moreData(f))

With your example, I would go with something like this (if I understand correctly):

// Make sure that we have a valid request:
// set it if it is not set already
if ( !$request ) {
    $request = $_SERVER['REQUEST_URI'];
}

I would also add: don't put comments in boxes. A "box" is hard to maintain and keep up: one spends more time fixing the "box" instead of keeping the comments current.

David