Was reading the slides from Aaron Patterson’s Magma Rails talk and noticed some pretty innocuous Rails code that, upon further reflection is the beginning of disaster for a growing application. As many other Rubyists are beginning to realize, spreading your application logic across only models and controllers leads to a mess. Let’s look at the code, understand why it’s bad, and create a better version.
Here’s the code to create a new user and email them a welcome note:
1 2 3 4 5 6 7 8 9 10
Simple enough, so what’s the problem? Let’s list out the things this class does:
- creates a new
Userinstance from form parameters
- saves the new
Userto the database
- Sends the user an email if the save was successful
- Renders the view
This is too many things. Every time we need to add something else that happens when a user is created, we will have to modify this methods. Further, any other part of the system that creates new users will have to duplicate this code (it’s not hard to imagine some sort of user import feature to create new users from some other system).
Fat Models, Skinny Controllers
The Rails Way is to put all of this into the model. Using the power of Rails, we could do this:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18
Is this better? Well, our controller is a lot simpler, and now just creates the user and renders the view. That’s pretty much all it should be doing. We’ve deferred our email to an
All we’ve done is move the problem somewhere else. We’ve also made testing our application a huge pain, because everywhere we create a
User instance for a test, we’ll fire off the
UserMailer, so we’d need to stub that our otherwise arrange for that code not to run, except when we test that code. Ugh.
So, in solving one problem, we’ve created another, giving us two problems, now:
Userclass is doing too much (even if we count all of ActiveRecord as just “one thing”)
- We’ve mixed up the concerns of creating instances of
Userobjects with creating new users of our application. The distinction might be subtle, but it’s important.
We can solve both of these problems using the single responsibility principle and by using one of Ruby’s most powerful and, sadly, underused features: creating a new class.
What we want is a single location for “someone new is using our application” and we don’t want that conflated with the creation of the class we use to store that user’s data in the database.
Since our new class is going to create new application users, let’s call it
ApplicationUserCreator. I know it’s the Kingdom of Nouns, but the more classes you have, the more specific their names have to be. We could use lambdas, but let’s keep things simple for now.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
Much better. Our
User class remains as it was originally - a class that holds data for the
USERS table and provides CRUD operations for it. Our controller is just as skinny in our second example - it launches the new user creation logic and renders the view. We have a new class which is custom built to hold the new application user logic.
These three classes are now very easy to test and very easy to understand; they all simply don’t do that much. Also, the test for our business logic (the test for
ApplicationUserCreator) is blazingly fast.
Resilience in the Face of Change
Where a design like this really shines is when we need to add new features to our app.
Suppose we want to do something different when creating administrative users. These users are still stored in the
USERS table, but we want to send them a different welcome email (perhaps admin users get a more security-conscious email).
We could start peppering
if user.admin? but that’s just wrong, too. The
UserMailer already does enough - it emails new application users a welcome email. It does not need to also email administrative users a security-related email. Let’s assume we’ve created
AdminUserMailer to handle that. We can also assume we have an
AdminUsersController that looks
1 2 3 4 5
UsersController, for now. Our
ApplicationUserCreator now needs to check if the new user is an admin. The quick and dirty path, which will get us into trouble, is to check inside that class, like so:
1 2 3 4 5 6 7 8 9 10 11 12 13
Well, this sucks. We now need to double our tests to handle the case where the new user is an admin. What happens when we add the next few feature? More
if statements and more complication. Lets decouple this class from the mailer it uses by allowing the mailer to be injectible:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
With this design, we can change mailers all we want, and won’t ever need to change
ApplicationUserCreator or its tests. We
should add a test that
UserMailer is the default and that we can inject our own mailer, but at that point,
ApplicationUserCreator is a completed class.
AdminUsersController now looks like this:
1 2 3 4 5
Not too bad. The classes involved in user creation are all dead simple and easy to test.
Suppose we had a third type of user creation scenario where we don’t want welcome emails to be sent at all? Not a problem:
1 2 3 4 5 6
I realize that Aaron’s code is just an example for a slide at a conference, but I can tell you from experience, that any time an authoritative source shows code to others, they take that as the “right way” to do things. It took the Java community years to stop writing code like this:
1 2 3 4 5 6
This code snippet is in every book on Java I’ve ever read, and I get why authors write it, but it’s Just Wrong. So is putting excessive business logic in your controllers or models.
What you can do
It’s very simple. When you are adding code to your Rails app, ask yourself two questions:
- Is this code about getting data in the right configuration for the view? If not, it does not belong in a controller.
- Is this code about manipulating data in the database? If not, it does not belong in the model.
Very little of the code you write goes in a controller or model, based on the above criteria. The code goes in some other class, possibly one you will have to create. It doesn’t go in a module that you include into your controller or model. It doesn’t go into a module that you
extend your model with at runtime, it goes into a class. That is the unit of code organization in an object-oriented language, so don’t be afraid to use it.