• Dissenter: Charles, you noob! Comments are evil. They can muddy up your code, nobody keeps them up to date and they're completely unnecessary. Write short, concise code and you don't need comments!

  • Me: You're absolutely right. Comments can be evil. But you're taking it too far! Not all comments are evil and the innocent are perishing in your genocide!

  • Dissenter: Hog-wallop! All comments are evil. You have simply not transcended to a higher level of programming.

  • Me: Perhaps. But, comments are not inherently evil. Their intent, the methods in which they are utilized, is the true evil! Comments don't kill code, programmer's kill code.

That was the envisioned conversation when you saw my bait title and wanted to come tell the new guy how comments are evil. Well, now that's out of the way let's get to the post. Comments, they aren't all bad.

stop blaming the comments for how you use them

Let me start off by saying that I am not encouraging the use of comments everywhere in your code. Unnecessary comments really are bad for your codebase. They're clutter, can be misleading and are a pain to keep up to date, so nobody keeps them up to date. And certainly if you're doing something like...

1 <?php
2 
3 $foo = 'foo';     // assign 'foo' to $foo
4 $foo .= 'bar';    // $foo is now 'foobar'
5 
6 $foo = someMethod($foo);     // call someMethod() on $foo

Please, PLEASE, stop doing this. Please.

That being said this isn't the comments fault. This is our fault. We are the ones that chose to put the comments there and create needless clutter. Stop blaming the comments and start looking in the mirror.

doc-block all the things

If it is a constant, class member, returns a value, takes a parameter, results in some side effect or is in some way a major block of code it should be documented.

Dissenter: Charles you fool! You want me to document getters?! Excuse me while I ignore you now.

Wait! Before you go, let me explain. I don't mean add a description to a getFoo() method that simply says "return $Foo". That's just plain silly! What you can do however is document the type being returned. This is highly important with a dynamically typed language, you don't really ever know what type a variable is until you do some kind of test on it. Here's an example of a, gasp, documented getter.

1 <?php
2     // inside a class somewhere...
3 
4     /**
5      * @return A Foo object that stores info about each doohickey
6      */
7     public function getFoo() {
8         // some code is here...it returns something
9     }

See, that's not so bad is it? You're not really adding a lot of clutter but you're giving users of your API the knowledge that when I call getFoo() I'm gonna get an object, and that object has the info about the doohickey I need. In addition a lot of modern IDEs can parse this and return useful auto-complete and documentation information based on these JavaDoc style tags.

finally, comment your cleverness

Sometimes, regardless of how well we name our variables and how elegant our code is we simply must have in-line comments. A lot of articles about writing comments will say this but then they never actually give a real-life example of a situation where you might need to write an in-line comment. Well, don't worry, I'm gonna give you what you need.

Below is a closure I use as an error handler before I can set up a more robust class-based error handler. It doesn't do much but trap errors and store the information in an array. Anyway, here's the closure, without any comments:

 1 <?php
 2 $errors = array();
 3 
 4 $errorCallback = function($severity, $message, $file = null, $line = null, $context = null) use (&$errors) {
 5 
 6     $normalizeSeverity = function() use ($severity) {
 7         $severityMap = array(
 8             E_WARNING => 'E_WARNING',
 9             E_NOTICE => 'E_NOTICE',
10             E_USER_ERROR => 'E_USER_ERROR',
11             E_USER_WARNING => 'E_USER_WARNING',
12             E_USER_NOTICE => 'E_USER_NOTICE',
13             E_USER_DEPRECATED => 'E_USER_DEPRECATED',
14             E_RECOVERABLE_ERROR => 'E_RECOVERABLE_ERROR',
15             E_DEPRECATED => 'E_DEPRECATED'
16         );
17         if (\array_key_exists($severity, $severityMap)) {
18             return $severityMap[$severity];
19         }
20         return 'E_UNKOWN_SEVERITY';
21     };
22 
23     $index = \count($errors);
24     $errors[$index]['severity'] = $normalizeSeverity();
25     $errors[$index]['message'] = $message;
26     $errors[$index]['file'] = $file;
27     $errors[$index]['line'] = $line;
28 
29     $unhandledSeverity = array(E_RECOVERABLE_ERROR);
30     if (\in_array($severity, $unhandledSeverity)) {
31         return false;
32     }
33 
34 };
35 
36 \set_error_handler($errorCallback);
This closure is also stored as a github gist. Please feel free to fork and make changes or improvements. If you make a really cool improvement let me know!

The closure is pretty simple really. It takes the appropriate arguments needed to satisfy the set_error_handler() callback. It also uses an array-by-reference outside of the closure to store the errors so we can access them later. I added a bit of severity normalization so error info doesn't have seemingly random integers in them. What may be "tricky", particularly to a new developer on the project, is the last little bit involving the $unhandledSeverity. What is that? Anybody can see that for E_RECOVERABLE_ERROR we're returning false and internal PHP error handling continues. But, why? Let's go ahead and take that out. I mean its a recoverable error, right? So, we should probably be doing something with it.

Wrong.

That little chunk of code is there for a very specific, very important reason. Did you know that if an improper type is passed to a function an E_RECOVERABLE_ERROR is raised? If we simply trapped that error we'd allow any data type to be stored where only a specific type should be. This certainly doesn't seem like a recoverable error to me. We need that line of code or else countless errors will occur later.

Comment it!

1 <?php
2     // ...
3 
4     // this is here to return errors on improper type passed as argument
5     $unhandledSeverity = array(E_RECOVERABLE_ERROR);
6     if (\in_array($severity, $unhandledSeverity)) {
7         return false;
8     }
9 };

I would definitely say that the added "clutter" is far outweighed by the extremely, useful information that the comment conveys and would be difficult to say in code. You won't find many in-line comments in my code but anytime I think something looks a little too clever or isn't perfectly clear as to what is going on I'll throw a simple comment for explanation.

wrapping it up

I hope you look at your code and see where maybe some documentation should be added, or even taken away. Remember that comments are available to us for a reason. Any tool can be utilized effectively to provide benefit, just as that same tool can be used for evil.