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
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
Then we perform the same operation for
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!