Unless readable else confused

I am on record as believing the unless...else construct in Ruby is an abomination. I thought I’d dig into that a little more, and explain why even the best-justified uses of unless...else make for questionable code construction.

Mislav makes as a good a case for unless...else as any I’ve seen.

unless response.redirect?
  # process response.body (the more common case)
else
  # follow response['location']
end

The purpose of using unless here is to put off the less common “special case” of redirection to later in the method. This is a good practice; the common case should always take the spotlight in a carefully constructed method.

This is trivially transformed into an if...else with negation, of course:

if !response.redirect?
  # process response.body (the more common case)
else
  # follow response['location']
end

I find this slightly more readable. But that’s not what this post is about, because the if...else version has the same fundamental problem as the unless...else version.

To explain what I’m talking about, allow me to present a dramatic reenactment of my thought process as I read either of these code examples:

  • Line 1: Looks like we’re doing something with responses.
  • Line 1 cont’d: Hm, there’s a special case I need to be mindful of involving redirects.
  • Line 2: Ah, but this here is the non special-case. Right? Yeah, right, this is the common case. But I’m still keeping that special case in the back of my mind.
  • Line 4: Aha, this is that special case that was mentioned at the beginning. What was the case again? Oh yeah, if the response is a redirect.
  • Line 4 cont’d: OK, now I understand what to do in the special case.
  • Line 5: Wait, what was the common case again?

The problem with this code is that despite trying to push off consideration of the special case to later on, it forces me to think about that special case right at the beginning. Then a part of my mind is waiting in suspense until the case is resolved. And since the condition and the handling are separated by the length of the common-case code, I may have forgotten what exactly the condition for the special case was by the time I get there.

All of this may happen in a matter of seconds, of course.

In Confident Code, I talk about maintaining the narrative flow of a method. I’d argue that the narrative here is unnecessarily convoluted.

It’s not always convenient to push special case processing to the end of a method. When it isn’t, I prefer to dispense with the special case quickly and completely before moving on to the common case. For the example given above, we could accomplish this with a guard clause:

return follow(response['location']) if response.redirect?

# process response.body (the more common case)

Here’s how I read this:

  • Line 1: Hmm, looks like we’re dealing with responses
  • Line 1 cont’d: OK, there’s a special case here involving redirects. Looks like that case is completely dealt with here.
  • Line 3: Ah, here we go, this must be the common case…

In this version I’m still confronted by the special case at the start of the method. But the guard clause gives me closure; once I’m done reading it, I feel like I can stop thinking about it completely.

Even if, unlike me, you find unless...else readable, the problem with it is that even in the best case it is used to separate special-case conditions from special-case handling. I don’t think this lends itself to readable (or refactorable) code. In revisiting the question, I find that my position is still to eschew the use of unless...else in all cases.

EDIT: To be clear, I’m only talking about unless...else here. I have no fundamental problem with unless used without an else clause.

10 comments

  1. I do a slightly different thing:

        follow(response[‘location’]) and return if response.redirect?
        # process response.body (common case)

    I find it a bit more readable than beginning the line with return.

    1. I can understand that point of view. My perspective is: an early return is potentially one of the most surprising things to find in a method, so I want it to be very, very obvious that a line may cause an early return. Hence, starting with the return.

      1. Ah, but an early return is only surprising if you don’t use early returns a lot – and it’s one of the few cases where I think the old-style one-statement-per-line adds clarity.  I’ve always been a fan of “straight line” programming, from the days before exception handling:

        if no_disk_space
          deal_with_it
          return
        end

        if unexpected_data
          deal_with_that
          return
        end

        do_the_real_thing
        for_30_lines_or_so

        I’m probably biased from reading my own code, but 20 years after I wrote it, I have no trouble following the narrative – the narrative is “skip over the guard clauses and find all the stuff in column 1”.  e.g.: https://gist.github.com/2259544

    2. BTW, there appears to be a bug in your code. The “and return …” will only be executed if the follow() returns a truthy value; I doubt that’s what you want.

      1. You are totally right. It was working because all the methods I used this with returned truthy stuff. So it was a hidden bug. Your idiom is better. Thanks!

      2. Ah, good to read about this. Never thought about that ” … and return” only returns if the first one does not return anything falsy. May have to check some code.

  2. I write special cases in the same way, but I think there’s still room for the if…else…end construct, because sometimes it means either/or instead of a common case and special case. You could argue that that’s what the ternary operator is for, and you’d be correct. But for some constructs, I find the ternary operator to be less readable. For example, when the ternary’s guard is a method ending with a question mark, I find the if…else…end construct more readable than having the two consecutive question marks.

Leave a Reply

Your email address will not be published. Required fields are marked *