Refactor to concepts

I strive for clean and readable code as much as I can. In this article I'll refactor a small snippet of code to make it as simple and understandable as possible. I like to call this technique - refactor to concepts.

Sometimes I see code like this:

class Library
  def audit
    book_loans.each do |book_loan|
      if book_loan.due_on < Date.today && book_loan.returned == false
        # do something interesting
      end
    end
  end
end

This makes my brain hurt and my eyes bleed.

Looking at it, you can work out what it's doing with a small amount of effort but when I read code I don't want to think about it. I want to be able to read the code and know what is going on with minimal effort, in fact, no effort at all if possible.

In the snippet above, we are looping over a series of book loans from a library. The loan of the book has a due date of when it should be returned and a flag that tells us if the book has been returned yet. Looking at the logic here we check that the due date is in the past (the due date is less than today's date) and we check that it hasn't been returned yet.

We are missing a named concept here. This boolean logic is describing when a book is overdue. Let's refactor this code one small step at a time. First, lets add a method that encapsulates this concept:

class Library
  def overdue?(book_loan)
    book_loan.due_on < Date.today && book_loan.returned == false
  end

  def audit
    book_loans.each do |book_loan|
      if overdue?(book_loan)
        # do something interesting
      end
    end
  end
end

OK, that looks better but I would like to improve the boolean logic and make that easier to read. Assuming we are using Rails, we can use the past? method provided by ActiveSupport and also the predicate version of returned like this:

# ...

def overdue?(book_loan)
  book_loan.due_on.past? && !book_loan.returned?
end

# ...

That's an improvement but notice the repeating use of book_loan? Passing an object to a method and then using that object's data is screaming at you to move this method to the object itself:

class BookLoan
  def overdue?
    due_on.past? && !returned?
  end
end

Now that we are here, I really don't like the negated returned? method call as part of the boolean logic. I think we are missing a concept here too. If a loaned book is not returned then you could say that it is still "out":

class BookLoan
  def out?
    !returned?
  end

  def overdue?
    due_on.past? && out?
  end
end

Our overdue? method is now very clean, easy to read and understand. It says what we mean in plain English. The loaned book is overdue when the due date is in the past and the book is still out.

A further improvement now that the overdue? method is on the BookLoan class is to make Library more expressive. We can also get rid of the conditional altogether by using Ruby's Enumerable#select method:

class Library
  def audit
    book_loans.select(&:overdue?).each do |book_loan|
      # do something interesting
    end
  end
end

Much better. In fact, I'd be tempted to move the filtering of overdue loans to it's own method as this is a concept too - overdue loans. The final code is now this:

class BookLoan
  def out?
    !returned?
  end

  def overdue?
    due_on.past? && out?
  end
end

class Library
  def overdue_loans
    book_loans.select(&:overdue?)
  end

  def audit
    overdue_loans.each do |overdue_loan|
      # do something interesting
    end
  end
end

That is a big improvement in my eyes!

I can hear some of you now: "That is way more lines of code!". Yep, it sure is and here's the thing - being a developer is all about trade-offs and balance. Here we have made our code much easier to reason about at the cost of increased lines of code. Personally I'd take that trade-off any day. Just compare the code before with after and remember this - code is read many more times than it is written and what we are doing here is helping our future self.

Many times in the past I've looked at code I've written and had literally no idea what it is supposed to be doing. Now I try to make the code as easy to understand as I can because I just want to read and understand the code first time without the need to use any brain power.

I try to follow these simple rules:

1. Name things well
2. Make things small
3. Avoid conditionals
4. Avoid nils

A side effect of this type of refactoring is that we now have small focused methods that has a single responsibility. These methods are now much easier to reuse and helps communicate intent.

It's easy to write hard to understand code but very difficult to write simple code. It's so helpful to have obvious code when you are trying to hunt down a bug or make changes to an existing system. I would encourage everyone to spend a little more time making the code they write super clear.