0

If I have a method to look for a valid email address in an array, that could either:

  1. Return the email address
  2. Return false if an email wasn't found.
  3. Throw an exception if an email was found, but it wasn't valid.

Should the method comment be written as though it would always work, or should it indicate that it might fail? For example:

/**
 * Attempts to return the email address from $data.
 * @param Array $data
 * @return Boolean
 * @throws My_Custom_Exception_InvalidEmailAddress
 */

vs

/**
 * Returns the email address in the $data.
 * @param Array $data
 * @return Boolean
 * @throws My_Custom_Exception_InvalidEmailAddress 
 *      Throw if an email key is found in $data, but the value is empty
 *      or if the value doesn't pass the Zend_Validate_Email check.
 */

Generally, I go by the rule of, if I've wrote some "throw new..." in the method, I'll write it more like the first example, because I know there's a good chance that the method won't do what the comment says, but, you can tell that by the @throws annotation, so it seems a bit unnecessary.

Is either comment style "prefered", or is it more just a case of be consistent throughout the project?

  • 2
    A good rule of thumb to follow is to code as if you couldn't use comments. A descriptive method name trumps a bad one with verbose comments every time. – Robbie Dee Jan 26 '16 at 11:16
  • 3
    Possible duplicate of Should code comments explain the control flow? – gnat Jan 26 '16 at 11:25
  • @RobbieDee, and this rule of thumb can (should) be extended to "only comment what is not covered by the code" – David Arno Jan 26 '16 at 12:10
3

Throwing an exception, by definition, means that the method doesn't return whatever it's supposed to be returning. If you have a @throws annotation (and you should, always!), then the fact that the promised return value might not happen is completely obvious. There is nothing gained by complicating the documentation to say "Tries to return" or anything like that.

What you should always write is, as exactly as possible, under which circumstances this will happen. it doesn't matter if the actual throw statement is in your code or in third-party code, the only difference is that you're usually a bit more certain of the exact circumstances if it's in your own code.

4

"Throws InvalidEmailAddress if an invalid email address is found" seems somewhat redundant.

Commenting what constitutes "invalid" would be a better use of the comment block, but otherwise comment the minimum necessary to understand what is intended, the more documentation you put in the more maintenance it will require as code changes.

  • Yes, but various tools can help... – Robbie Dee Jan 26 '16 at 11:14
0

It is, as always, mostly about keeping it consistent.

One could argue that if you throw a checked FoundInvalidEmailAddressException you won't even need the throw because it provides duplicated information that the compiler will tell you anyway, but I'd write it like this:

/**
 * Returns the email address in the $data.
 * @param  Array $data //what format do you expect data to be? 
 *         eg. [username, password]? [username, email]?
 * @return Boolean true if the email is found and valid, false otherwise
 * @throws InvalidEmailAddressException If there is an email for $data, 
 *         but it fails validation. An email is invalid if...
 */

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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