There are a pair of practices in the Ruby on Rails world that some people advocate as applications of good software design. I think they are anti-patterns.
- bypassing the way controllers transfer data to the views;
- abusing before_actions in controllers.
This article aims to criticize the first one, but only after some considerations on code readability, style conventions and Rails internals.
A note on code readability
I believe in the importance of code readability, as it affects the maintainability of a project and eases the introduction of new team members. Sometimes maximizing code readability will require sacrifices on other fronts, just like for many other elements of software design. Other times, however, the benefits of readable source code plainly outweigh the trade-offs.
Code readability mainly refers to how the written source code is structured and formatted. Things like whitespace, comments and decomposition are all very important, and so are naming conventions. Names should be clear, expressive and univocal, so that it can be possible to immediately grasp their meaning. Such is the importance of this element, that Ruby even uses a strict naming convention to define a variable’s scope: prefixes for
A note on encapsulation and accessors
Let’s look at what is considered bad and good in Rubyland.
Ruby doesn’t provide direct access to an object’s internal state.
Yes, we can open objects at runtime and do pretty much whatever we want with them, but instance variables are meant to be accessed through accessor methods. For example:
1 2 3 4 5 6 7 8 9 10 11 12
class Foo attr_reader :bar def initialize(bar) @bar = bar end end foo = Foo.new('bar_value') baz = foo.bar puts baz # => 'bar_value'
This behaviour is an expression of a concept defined, in Object Oriented Programming, as encapsulation.
On the internet there are historic circles, famous for their focus on software design patterns, where we can find discussions on its definition, interpretation and actual usefulness. They are worth reading.
Why is this important?
It’s important because by interacting with an interface, we don’t need to know about the implementation details. As long as a class maintains a stable interface (and consistent return values), it can change its implementation without breaking the rest of the application. Not only does this allow that class to add features and improvements in isolation, but it creates a clear contract between that class and its clients: the other objects in the program.
My opinion is that a flexible encapsulation helps write better code. However, since Ruby enforces it anyway, the argument is moot. In Ruby, this kind of encapsulation is a good thing.
Let’s look at a ruby example where encapsulation is violated. If you find yourself doing something like this, and you don’t have a good reason, then you are doing a bad thing:
1 2 3 4 5 6 7 8 9
class Foo def initialize(bar) @bar = bar end end foo = Foo.new('bar_value') baz = foo.instance_variable_get(:@bar) # don't do this baz = foo.instance_eval('@bar') # don't do this either
What’s happening is that the rest of the program is invading
Foo’s private space. I say it’s a bad thing because the
Foo class is leaking its internal state without an API.
Of course, there are situations where using this kind of variable access is fine (although we could probably argue about it), for example:
- when variables are being dinamically defined (metaprogramming, and typically the object does it internally),
- in test cases where you need to verify the value of an instance variable.
Rails controllers, views and @instance variables
Now, let’s talk about Rails controllers.
Rails controllers contain methods (actions) that setup the application state necessary to render the corresponding views. This state is represented as
@instance variables of the controller, which will be available to the ruby code within the view files.
At first glance, this might look similar to the bad thing described above. Let’s see why it isn’t.
A Rails view doesn’t have direct access to its controller’s instance variables. Rather, the controller’s variables are transferred to the view renderer.
Before rendering a view, instances of
ActionController::Base will store their (non-protected) instance variables in a hash, here (rails 4.0.2). Then, the hash will be used to initialize an instance of
ActionView::Base (responsible for rendering the views), here (rails 4.0.2). Finally,
ActionView::Base will use the hash passed to its constructor to create its own set of instance variables, here (rails 4.0.2).
What Rails controllers do is very different from the bad thing.
They create a new object and pass a set of parameters to its
initialize method. The object then uses Ruby’s metaprogramming features to build its internal state from these parameters.
It could be argued that this doesn’t change much, as we are still leaking state: a controller could need to use instance variables for something different than sending data to the view.
As I see it, though, when using Rails controllers and views we are not dealing with an object graph, where objects should interact with each other through their public interfaces. Rather, controllers and views are a system to be used as a whole. The data flow is automatic and invisible.
The Rails convention and code readability
The fact that views have access to the same instance variables of the controller is a Rails convention.
The question we should ask is: does this convention have any effect on code readability? I would say yes, and a very good one.
Consider this html.erb partial:
1 2 3 4 5 6 7 8 9
<ul> <% @articles.each do |article| %> <li class="<%= css_class_for article.category %>"> <strong>article.title</strong> <%= link_to 'edit', edit_article_path(article) if admin %> <%= render 'article_details', article: article %> </li> <% end %> </ul>
We can see immediately that
@articles is an instance variable, and this tells us that it comes from the controller (declaring instance variables in views is bad). The fact that it comes from the controller means that, at the very least:
- its content is static, it’s the final product of a series of operations, ready to be consumed by the view,
- we know (almost) exactly where it was defined: the controller or one of its ancestors.
Compare this visual cue with the other chunks of embedded Ruby code.
In this partial
article is a local variable scoped in the block. However, on line 6 we render another partial and pass to it a local variabled named
article. In that new partial
article will be a local variable received by its parent, not a block variable.
css_class_for has probably been defined in a helper class, but there is a chance that it was defined in the controller as a helper method.
admin? Is it an helper method? Was it defined in the controller? Or is it a local variable passed to this partial by its parent view?
The truth is that
@instance variables stand out and are quickly recognizable. They are a very clear way to separate what was built in the controller (i.e. by quering the DB), and what is either locally scoped (partial locals, block variables) or dynamically calculated (helper methods).
In order to fully understand that small snippet, we need to look at other files (the controller and its ancestors, the helpers, the parent template, etc). This is where naming conventions make a huge difference, and so does that little
@ character in
That said, please note that these consideration are about the readability of the source code.
In no way I want to suggest that partials are bad. Organizing the views in reusable and purpose-isolated partials is the best way to keep the view layer organized and maintainable.
Also, I have to mention that relying on
@instance variables can make it hard to keep partials reusable, because that means that all controllers’ actions that will render those partials will need to set the correct state. This is why we assign locals.
The anti-pattern: controller accessors
(rant mode: on)
The story goes like this: Rails views have direct access to controllers’ instance variables. Since directly accessing another object’s instance variables is bad, it is necessary to fix the problem by adding a public interface to controllers.
There are several ways to accomplish this.
For example, the decent_exposure gem was created with this use case in mind (by the way, the author wrote a very good article on the importance of object interfaces). Another technique is to use helper_method, which is useful when some logic that belongs in the controller needs to be made accessible to the views.
This means that it can also be used to wrap variables in getters:
1 2 3 4 5 6 7 8 9 10 11 12 13 14
# GET /articles def index @new_article = Article.new end def articles @articles ||= Article.order(title: :asc) end def new_article @new_article end helper_method :articles, :new_article # don't do this
As said at the beginning of the post, I consider this practice an anti-pattern, and not just because the premise is based on incorrect information (see previous paragraph on Rails internals).
First off, I don’t think it’s actually useful as there is no real problem to solve. Controllers and views are not to be used as seprate entities in an object graph and don’t need to interact through a public interface.
We don’t directly instantiate controllers, view renderers, or include view helpers.
When we call their methods (well, send them messages), we don’t use a reference to their current instance (a variable).
Rails makes them interact under the hood and we, as developers, just use the final product.
Compare this with how we manage models in our code. We instantiate them and reference them with variables, to which we can directly send messages. The way they are treated in the framework is different and, in fact, Rails models expose a very rich public interface.
The point is that there is a profound difference between objects that are meant to be managed by the user (models, presenters), and objects automatically managed by the framework (controllers, view renderers).
In addition to being not useful, I believe that this practice negatively affects code readability.
Using accessors instead of instance variables requires to replace things like
I realize this is a matter of personal taste, but I find it makes the code less clear. Not just because of the purely aestetic clarity of
@this compared to
this, but because the latter can’t be distinguished from local variables and helper methods.
When I need to understand the code in a partial, I look for the definitions of the variables and methods it uses. This means I will open other files and look for the the right portions of code.
Working with instance variables (at least at the root level of the view hierarchy) makes things much easier, because the location of their definition is almost certainly the controller or one of its ancestors.
I also think this practice introduces unnecessary complexity. Thanks to the
@ character, instance variable names are automatically namespaced. With accessor methods, on the other hand, we add a new category of entities whose names could potentially conflict with local and block variables.
Of course the problem is trivial, and we could easily solve it by picking more precise names in the partial files, but without controller accessors it wouldn’t be an issue in the first place.
(rant mode: off)