I postulate that comments are a code smell.
Craig Buchek suggests a possible counterexample:
# Let this raise its exception if the fields don’t exist as expected.
user = c.get_setting(‘username’)
How else could one express the intention here, he asks?
I say, the output of a function should be expressed in its return value. Exceptions are cheating — they bust out of normal flow control. That’s a surprise, and surprises are to be avoided. No surprises, no comments needed.
Why is it throwing an exception? get_setting must be mucking around in the environment. The fetching of a setting from outside a program is a side effect, a dependency on the world around program execution, a bummer. It’s necessary, so let’s do this carefully.
Instead of throwing an exception, get_setting could return the failure information to its caller. This is a classic use for Either. In Scala:
def get_setting(settingName: String): Either[Error, String]
This method signature reports that you will get back the setting you want OR information about the error. (The convention is that Either contains the desired value on the right or the failure information on the left. In the type parameter list, that puts the failure type first, which is unfortunate.)
Then what does the calling function do with that Either it gets back? We don’t like a whole bunch of “if it worked, do this” statements; that’s not much cleaner than try/catching. Let’s fill in some hypothetical code around Craig’s snippet.
def getAuthorizationLevel(c: SettingGetter,
authorizer: Authorizer): AuthLevel = {
# Let this raise its exception if the fields don’t exist as expected.
user = c.get_setting(‘username’)
authorizer.getAuthorizationLevelForUser(user)
}
Here, the authorizer method can also throw an exception. If we replace exception-throwing with explicit error handling using Either, we get:
def getAuthorizationLevel(c: SettingGetter,
authorizer: Authorizer): Either[Error, AuthLevel] = {
val possibleUser = c.get_setting(‘username’)
val possibleAuth = possibleUser.right.map{ user =>
authorizer.getAuthorizationLevelForUser(user) }
possibleAuth.joinRight
}
OK. This is cryptic if you aren’t used to working with Either. (I had to look up joinRight.) What’s happening?
right.map says, “If we have a string, replace it with the output of this function.” We take that string (user) and turn it into… what? Here’s the signature for the next method call:
def getAuthorizationLevelForUser(user: String) : Either[Error, AuthLevel]
This output will replace the user string. We took possibleUser, which was either an error or a string, and now in possibleAuth we have either an error (from user) or an error (from auth) or an AuthLevel. It’s Either[Error, Either[Error, AuthLevel]]. Yuck!
joinRight says “If we have an Either on the right, return that instead of this one.” We get an Either[Error, AuthLevel]; the Error could be from getting the user setting or from getting the authorization level. joinRight scrunched the two errors into the same place.
That feels like a lot of work to avoid throwing an exception. Yet, the comment isn’t necessary because the possibility of failure is explicit in the return values. There’s no if/else or try/catch blocks; instead, processing based on the user setting is moved into the context of the Either, so that we fetch the auth level only if we got that user string.
Now the possibility of failure is returned explicitly to the caller of getAuthorizationLevel. If they want to report an error back up the chain, great. If they log the error and then use a default authorization level, even better. In case you’re curious, that would look like this:
val possibleAuth = getAuthLevel(settingGetter, authorizer)
possibleAuth.left.foreach(error => log(error))
val authLevel: AuthLevel = possibleAuth.right.getOrElse(AuthLevel.GUEST)
Errors are data, too. Return them to your caller in the same flow of control as the data you were hoping to supply.
———————————–
Scala code for this post: https://gist.github.com/jessitron/5788968
@marioaquino agrees with me: http://marioaquino.blogspot.com/2013/06/styles-of-handling-conditional-logic.html
So does @heathborders: http://heath-tech.blogspot.com/2013/06/all-smart-kids-are-writing-blog-posts.html
and @Adkron, although he loves OO more: http://dirtyinformation.com/blog/2013/06/15/exceptional-comments/
… now I hope @CraigBuchek will chime in with an opposing point of view because we’re running out of argument here.
and here’s a timely one from @josefusbarnabus that agrees comments are evil: http://heath-tech.blogspot.com/2013/06/all-smart-kids-are-writing-blog-posts.html
and alonzophoenix says types are better than comments: “type annotations—like pretty much everything else in programming language source code—are best thought of as more for human consumption than for the compiler.” http://archontophoenix.blogspot.com/2013/04/no-comment.html