If I have a method to look for a valid email address in an array, that could either:
- Return the email address
- Return false if an email wasn’t found.
- 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?
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.
“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.
1
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...
*/