Josh Adams | @knewter | josh@isotope11.com
Every class should have a single responsibility.
Every class should have a singleresponsibilityreason to change.
# Persistence
class Person < ActiveRecord::Base
# associations
has_many :arms
# round-about sql
scope :you_can_trust, where('age < 30')
# business logic
def favorite_arm
arms.first
end
end
So ActiveRecord has tentacles into lots of your system by default. It's both a blessing and a curse.
It makes it extremely easy to build a decent application without taking the time to tease out your actual application domain.
We aren't going to fix everything with it, but knowing it should make us be extra-careful with the rest of our system at least.
"My application got much harder to maintain after six months"
- 50% of rails developers everywhere
Things they're not meant to do (but often end up doing):
Things they are meant to do (but rarely do):
Bob Martin's 2011 Ruby Midwest Keynote is eye opening in this regard.
Consequently...
"I need to verify that my application logic works end to end...I know, I'll click through the browser and wait on my tests for a long-ass time!"
Build out domain objects that represent your application, and don't shoehorn everything into ActiveRecord and the controllers
A good first-approximation of good design is to make sure that your controllers don't directly interact with your persistence layer (or do so as minimally as possible). I like to do this by defining Command objects for my controllers to collaborate with.
# General Ledger accounts
class LedgerAccount
end
class Student < ActiveRecord::Base
def checking_account;end
def savings_account;end
end
class CreditManager
# ...
def transfer_funds_from_checking_to_savings(student, amount)
end
def transfer_funds_from_savings_to_checking(student, amount)
end
# ...
end
It's a webapp, so we're going to have a form on a webpage to take in user input. That's going to submit to a controller. The controller might look like:
class TransfersController < LoggedInController
def create
credit_manager = CreditManager.new
student = current_person
amount = BigDecimal(params[:amount])
if student.checking_account.balance > amount
if credit_manager.transfer_funds_from_checking_to_savings(student, amount)
flash[:success] = "Transfer successful."
else
flash.now[:error] = "Sorry, something went wrong."
end
else
flash[:error] = "Sorry, not enough money."
end
redirect_to bank_path
end
end
Often the next level of sophistication in a Rails app looks like this:
Complaints
class StudentTransferCommandsController < LoggedInController
def create
transfer = StudentTransferCommand.new(params[:student_transfer_command])
transfer.student_id = current_person.id
transfer.on_success = method(:on_success)
transfer.on_failure = method(:on_failure)
transfer.execute!
end
def on_success
flash[:success] = "Transfer successful."
redirect_to bank_path
end
def on_failure
flash[:error] = "Invalid transfer."
redirect_to bank_path
end
end
require 'active_model'
class ActiveModelCommand
include ActiveModel::Validations
include ActiveModel::Naming
include ActiveModel::Conversion
# This is so that activemodel acts like we want in the form
def persisted?
false
end
end
require_relative 'active_model_command'
require_relative '../validators/positive_decimal_validator'
class StudentTransferCommand < ActiveModelCommand
attr_accessor :amount, :direction, :student_id, :on_success, :on_failure
validates :direction, presence: true
validates :student_id, presence: true, numericality: true
validates_inclusion_of :direction, in: ["savings_to_checking", "checking_to_savings"]
validates :amount, positive_decimal: true
def initialize params={}
@amount = BigDecimal(params[:amount]) if params[:amount]
@direction = params[:direction]
@student_id = params[:student_id]
@on_success = lambda{}
@on_failure = lambda{}
end
# The transfer knows what to call on credit manager based on its direction
def transfer_method
case direction
when "savings_to_checking"
:transfer_credits_from_savings_to_checking
when "checking_to_savings"
:transfer_credits_from_checking_to_savings
end
end
def student
Student.find(student_id)
end
def credit_manager
CreditManager.new
end
def execute!
on_failure.call() unless valid?
success = credit_manager.send(transfer_method, student, amount)
if success
on_success.call()
else
on_failure.call()
end
end
end
class PositiveDecimalValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
unless value.present?
record.errors[attribute] << "Must be present"
return false
end
unless value.is_a?(BigDecimal)
record.errors[attribute] << "Must be a BigDecimal"
return false
end
record.errors[attribute] << "Must be positive and non-zero" unless value > BigDecimal('0')
end
end
require 'test_helper'
require_relative '../../app/models/student_transfer_command'
describe StudentTransferCommand do
subject { StudentTransferCommand.new }
it "requires valid amount" do
subject.wont have_valid(:amount).when(nil)
subject.wont have_valid(:amount).when(0)
subject.wont have_valid(:amount).when(BigDecimal('-1'))
subject.wont have_valid(:amount).when('asdf')
subject.wont have_valid(:amount).when('123')
subject.must have_valid(:amount).when(BigDecimal('1'))
end
it "requires valid direction" do
subject.wont have_valid(:direction).when(nil)
subject.wont have_valid(:direction).when("foo")
subject.must have_valid(:direction).when("savings_to_checking")
subject.must have_valid(:direction).when("checking_to_savings")
end
it "requires valid student_id" do
subject.wont have_valid(:student_id).when(nil)
subject.wont have_valid(:student_id).when("foo")
subject.must have_valid(:student_id).when(1)
end
it "knows the type of credit manager transfer to execute based on its direction" do
subject.direction = "checking_to_savings"
subject.transfer_method.must_equal :transfer_credits_from_checking_to_savings
subject.direction = "savings_to_checking"
subject.transfer_method.must_equal :transfer_credits_from_savings_to_checking
end
it "executes the appropriate transfer when #execute! is called" do
amount = BigDecimal('5')
subject.amount = amount
method = :meth
student = mock "student"
credit_manager = mock "credit manager"
subject.expects(:student).returns(student)
subject.expects(:credit_manager).returns(credit_manager)
credit_manager.expects(method).with(student, amount).returns(true)
subject.expects(:transfer_method).returns(method)
subject.execute!
end
end
$ rake test TEST=test/unit/student_transfer_command_test.rb
StudentTransferCommand
PASS test_0002_requires valid direction (0.04s)
PASS test_0001_requires valid amount (0.01s)
PASS test_0003_requires valid student_id (0.01s)
PASS test_0004_knows the type of credit manager transfer to execute based on its direction (0.00s)
PASS test_0005_executes the appropriate transfer when #execute! is called (0.00s)
Finished in 0.06093s
# ^^^^^^^^ that would be the interesting part
We were also trying to adhere to the tell, don't ask philosophy. We told the Command what we'd like to do on success and failure, and then deferred to it after execution.
By having callbacks, we can more easily reuse this command inside of other commands, other places.
class LockersController < LoggedInController
def show
locker_sticker_links = load_locker_sticker_links
render 'show', locals: { locker_sticker_links: locker_sticker_links }
end
#...
end
I've seen too many projects have too many bugs based on helpers that modified instance variables. This way, it's simple to see what happens to a given variable by just following it through the application flow, without being scared some helper call might change it. Only things that it's passed to would even have a chance.
Writing code this way is more verbose in the short run.
It's worth it, due to the additional certainty that the code is correct. People don't test controllers. They might integration test the happy path, but it's pretty rare to see edge cases covered in controllers. Drastically simpler to handle it in the unit tests, and then just integration test the happy path.
If you mock all of your collaborators, then you aren't really testing your app and you won't catch it when your protocol changes.
I never said you didn't need to also do integration testing, but if you've solidly tested your units then the integration tests (which will always be slower) can be a lot more terse and you can test the integration without testing the even-slower bits (driving a browser).
Scenario: Students bid each other up on an auction
Given an auction exists
Then the auction's current bid should be 0
Given two students exist with sufficient credits
When the first student bids on the auction for $2.00
Then the auction's current bid should be $2.00
When the second student bids on the auction for $1.00
Then the auction's current bid should be $2.00
When the second student bids on the auction for $3.00
Then the auction's current bid should be $3.00
And the first student should have a message informing him he's been outbid
class Auctions < Spinach::FeatureSteps
#...
When 'the first student bids on the auction for $2.00' do
bid(@student1, '2')
end
Then 'the auction\'s current bid should be $2.00' do
validate_current_bid('2')
end
#...
def bid(person, amount)
amount = BigDecimal(amount)
bid = BidOnAuctionCommand.new({
auction: @auction,
person: person,
amount: amount,
credit_manager: @credit_manager
})
bid.execute!
end
def validate_current_bid(amount)
@auction.current_bid.must_equal BigDecimal(amount)
end
end
So you can go on to test a ton of different edge cases in your integration tests, and not be concerned about how slow they are because browsers / full stack are slower.
But this isn't idiomatic Rails
Previously, idiomatic Rails hasn't been very OO-friendly. There's a sea-change happening presently, and in a year this'll be far closer to idiomatic Rails than the 15-minute-blog presentation is.
...although DHH will probably talk shit about it.