In Progress
Unit 1, Lesson 1
In Progress

Complex Predicate

Video transcript & code

Much of the business logic in our applications boils down to predicates. Predicates are just questions that have yes or no answers. Does the bank account have enough money in it to cover a charge? Has the user logged in within the last year? Do you want fries with that?

Some questions are more complex than others, and may depend, in turn, on answering several other simpler questions first. Here's an example of a moderately complex predicate, which was captured in the wild and donated by David Verhasselt.

class Reminder
  attr_reader :reminder_at, :completed, :last_reminded_at

  def initialize(completed: false, reminder_at:, last_reminded_at: nil)
    @completed = completed
    @reminder_at = reminder_at
    @last_reminded_at = last_reminded_at
  end

  alias completed? completed

  def alert_due?
    [
     !completed?,                                                    # Todo needs to be incomplete
     reminder_at <= Time.current,                                    # Check if first reminder to be sent is due
     last_reminded_at == nil || last_reminded_at <= 23.hours.ago,    # Check if last reminder sent was yesterday
     reminder_at.hour <= Time.current.hour,                          # Check if the hour is now or has passed
     reminder_at.min <= Time.current.min,                            # Check if the minute is now or has passed
    ].all?
  end

end

This predicate lives in a class called Reminder. The purpose of Reminder is fairly self-explanatory: it keeps track of reminders. The predicate we are looking at determines whether it is time to alert the user about the reminder.

We want to make some changes to the logic in this method. But first, we'd like to refactor it, both to increase our understanding of the method, and to make the upcoming modifications go more smoothly. As Kent Beck says, first make the change easy, then make the change. Fortunately, we've written some characterization tests that will verify that the logic of the method hasn't changed, so we can safely begin to refactor.

Let's start off with some low-hanging fruit. The method is principally made up of a series of logical tests, with a comment after each one explaining the test in domain terms. The first test doesn't even need a comment, because it's self-explanatory: it checks whether the reminder has been marked completed. On the other hand, farther down we see tests that are comparing minute fields inside of time objects.

There is a common code smell in this code: it deals in inconsistent levels of abstraction. Let's fix that. We're going to go down this list of checks, replacing each one with a method call. In the process, we'll remove the need for comments by giving the extracted methods intention-revealing names.

We extract the first complex check into a method named current_time_is_past_due?.

We extract the next test into a method called not_sent_yesterday?. Normally, we prefer to avoid predicate methods with names that start with not, since we can always negate the result if we need to. However, when we are refactoring we try to do one thing at a time, and this method name matches the current intent of the code it encapsulates. We make a note to switch this around to a positive predicate in our next round of refactorings.

The next couple of tests exist because of how alerts are supposed to work. If a reminder is set on the <>10th of November at 9:30, and it is not marked completed, then it should trigger an alert at that date and time. It should also trigger an alert on the <>11th of November at the same time of day or later, and on the 12th, and so on.

We extract one of these tests into a method named hour_is_past?.

Then we perform the same operation for minute_is_past?.

We are left with code which is considerably more intention-revealing, and has no need for clarifying comments.

def alert_due?
  [
   !completed?,
   current_time_is_past_due_time?,
   not_sent_yesterday?,
   hour_is_past?,
   minute_is_past?
  ].all?
end

def current_time_is_past_due_time?
  reminder_at <= Time.current
end

def not_sent_yesterday?
  last_reminded_at == nil || last_reminded_at <= 23.hours.ago
end

def hour_is_past?
  reminder_at.hour <= Time.current.hour
end

def minute_is_past?
  reminder_at.min <= Time.current.min
end

Now that we've gone down the list, we return to the negatively-named not_sent_yesterday? method. We invert its logic, and rename it to sent_yesterday?. Then we go back to its call site, update the name, and prefix it with a bang to negate the result. This line is now consistent with the first predicate in the list, where we ask if the reminder has been completed and negate the result.

def alert_due?
  [
   !completed?,
   current_time_is_past_due_time?,
   !sent_yesterday?,
   hour_is_past?,
   minute_is_past?
  ].all?
end

# ...

def sent_yesterday?
  last_reminded_at != nil && last_reminded_at > 23.hours.ago
end

Extracting all of these methods has made it easier to notice another oddity in this code: all of the tests are bundled into an array, which is then queried for whether it is all true. We're not sure what the origin of this stylistic choice is, but it's a bit un-idiomatic. We get rid of the array, and instead chain all of the true or false tests together with the && operator. While it probably won't make much of a difference, this also has the side effect of making the method a little more efficient. This is because instead of always executing every test, it will now only execute up to the first false result and then return.

def alert_due?
   !completed? &&
   current_time_is_past_due_time? &&
   !sent_yesterday? &&
   hour_is_past? &&
   minute_is_past?
end

What we are left with is a pattern called a composed method. This pattern, from the book Smalltalk Best Practice Patterns, describes a method which is implemented entirely in terms of a series of calls to other methods, all of which are at the same level of abstraction.

We could easily stop here. This code is idiomatic and expressive. But there's one potential problem with it, and it's a totally non-obvious one.

Imagine one day we are trying to debug an issue with the program, and in the process we need to understand why a reminder isn't triggering an alert. So we have our scenario all set up, and we require a debugger and drop in a manual breakpoint.

r = Reminder.new(reminder_at: 2.days.ago + 5.minutes)
require "byebug"
debugger
r.alert_due?                          # => false

Then we execute our test script. The breakpoint drops us in right where we want to be. We give the step command to step in to the method about to be invoked, and find ourselves in a familiar method. "OK" we think; "now we'll just step one line at a time and see how far we get".

So we enter the next command and… Ho! Well then. I guess we are done! How did that happen?

 $ ruby debug.rb

[33, 42] in debug.rb
   33:
   34:   def minute_is_past?
   35:     reminder_at.min <= Time.current.min
   36:   end
   37: end
   38:
   39: r = Reminder.new(reminder_at: 2.days.ago + 5.minutes)
   40: require "byebug"
   41: debugger
=> 42: r.alert_due?                          # => false
(byebug) s

[11, 20] in debug.rb
   11:
   12:   alias completed? completed
   13:
   14:   def alert_due?
   15:      !completed? &&
=> 16:      current_time_is_past_due_time? &&
   17:      !sent_yesterday? &&
   18:      hour_is_past? &&
   19:      minute_is_past?
   20:   end
(byebug) n

It turns out that the debugger treats the entire chained logical expression as a single line for the purpose of stepping through the code. This is not very useful to us.

If we care about ease of debugging in complex predicates, there's another way we can write our logic. Instead of chaining our tests together with logical operators, we structure the code as a series optional early returns. Return false if the reminder is already completed. Return false if it is not yet due. And so on down the line. Finally, when all the reasons that this predicate might not return true have been exhausted, we return true.

def alert_due?
   return false if     completed?
   return false unless current_time_is_past_due_time?
   return false if     sent_yesterday?
   return false unless hour_is_past?
   return false unless minute_is_past?
   true
end

Let's toss this new version into the debugger. This time, we are able to step through the logic one line at a time. Each time a line fails to return, we know that the condition was not satisfied. Finally, we see that the last test causes an early return, and we have now increased our understanding of the current scenario.

 $ ruby debug2.rb

[34, 43] in debug2.rb
   34:
   35:   def minute_is_past?
   36:     reminder_at.min <= Time.current.min
   37:   end
   38: end
   39:
   40: r = Reminder.new(reminder_at: 2.days.ago + 5.minutes)
   41: require "byebug"
   42: debugger
=> 43: r.alert_due?                          # => false
(byebug) s

[10, 19] in debug2.rb
   10:   end
   11:
   12:   alias completed? completed
   13:
   14:   def alert_due?
=> 15:      return false if     completed?
   16:      return false unless current_time_is_past_due_time?
   17:      return false if     sent_yesterday?
   18:      return false unless hour_is_past?
   19:      return false unless minute_is_past?
(byebug) n

[11, 20] in debug2.rb
   11:
   12:   alias completed? completed
   13:
   14:   def alert_due?
   15:      return false if     completed?
=> 16:      return false unless current_time_is_past_due_time?
   17:      return false if     sent_yesterday?
   18:      return false unless hour_is_past?
   19:      return false unless minute_is_past?
   20:      true
(byebug) n

[12, 21] in debug2.rb
   12:   alias completed? completed
   13:
   14:   def alert_due?
   15:      return false if     completed?
   16:      return false unless current_time_is_past_due_time?
=> 17:      return false if     sent_yesterday?
   18:      return false unless hour_is_past?
   19:      return false unless minute_is_past?
   20:      true
   21:   end
(byebug) n

[13, 22] in debug2.rb
   13:
   14:   def alert_due?
   15:      return false if     completed?
   16:      return false unless current_time_is_past_due_time?
   17:      return false if     sent_yesterday?
=> 18:      return false unless hour_is_past?
   19:      return false unless minute_is_past?
   20:      true
   21:   end
   22:
(byebug) n

[14, 23] in debug2.rb
   14:   def alert_due?
   15:      return false if     completed?
   16:      return false unless current_time_is_past_due_time?
   17:      return false if     sent_yesterday?
   18:      return false unless hour_is_past?
=> 19:      return false unless minute_is_past?
   20:      true
   21:   end
   22:
   23:   def current_time_is_past_due_time?
(byebug) n

Now, I won't say that all predicate methods should be written this way. But when they are like this one, testing five different criteria at once, a series of explicit short-circuits can make the code a lot easier to debug, should we ever need to.

And that's all for today. Happy hacking!

Responses