The Perfect Weapon

In this chapter, our intrepid hero writes slightly-more-tolerable code.

This is a follow-up to my earlier article Making a mockery of ActiveRecord. That seemed to prompt a fair amount of useful discussion. One of the most interesting can be found here, on James Mead's blog. He is more polite about it than this, but to summarize: "Your tests would be easier to write if your code didn't suck."

One way to think about testing (particularly in a message-oriented language like Ruby) is as a weak mathematical proof. Don't take that analogy too far, since we aren't really formally proving anything. Consult a medical professional before using my blog, or any other drug.

Suppose we have a method called 'a', and it invokes the methods 'b', 'c', and 'd' to get its job done. If we have already written tests for 'b', 'c', and 'd' showing that they behave correctly when given a certain set of inputs, all that is left is to show that 'a' gives them that input. This is the concept that lets us use mocks, and still be comfortable that our code is probably correct. We don't need to re-test method 'b' every time it is called. We already know that it works.

What does this have to do with my previous article? James indirectly pointed out to me that I hadn't written a Composed Method, and I was making life hard for myself.
Here's what the code looked like when I wrote the article:

 1 def generate_email_messages
 2   people = self.recipients
 3   Message.transaction do
 4     people.each do |person|
 5       unless person.campaigns.include?(self.campaign)
 6         person.subscriptions.create :campaign => self
 7       end
 8       em = self.email_messages.create :person => person, :message => self, 
 9         :direction => 'mt', :sent_at => Time.now.utc, :body => self.body
10       unless em.valid?
11         raise RuntimeError.new("Unable to ..blah.. for #{person.email_address}.")
12       end
13     end
14   end
15   people.size
16 end

This isn't too bad, but it definitely has some flaws. On line 5, I am reaching out 'through' Person, and interrogating it to decide whether I need to create a Subscription. Clearly, the Person model should be deciding whether or not it needs a subscription.

The same thing happens again on line 10. I reach through the email_messages association, and then raise an error if the creation failed. The EmailMessage model should contain the knowledge of what constitutes an error. This is particularly true since this example is greatly simplified, and the real model uses acts_as_state_machine with conditional validations. As your models grow in complexity, the importance of having the functionality in the right place rises.

Here's what the code looks like now.

 1 def generate_email_messages
 2   people = self.recipients
 3   Message.transaction do
 4     people.each do |person|
 5       person.subscribe_to(self.campaign)
 6       EmailMessage.generate_message_for(self, person)
 7     end
 8   end
 9   people.size
10 end

There are new methods on Person and EmailMessage that encapsulate the necessary steps. At first glance, those methods look trivial. Neither is longer than three lines. However, they can now be tested in isolation, and work the same way with both existing and unsaved objects. ActiveRecord's association proxies (email_messages, campaigns, etc) are powerful tools, but they can also distract you from the actual goal, which is always to write readable programs with as few defects as possible.