Today I’m going to refactor acts_as_list Rails library. It allows to treat Rails model records as part of an ordered list and offers methods like
Step 1: .acts_as_list introduction
.acts_as_list is available as a class method in
acts_as_list gem is loaded. Here’s an excerpt from
ClassMethods module is customary in Rails, but it’s not a requirement to be familiar with it to read this article. All you need to know is that
.acts_as_list is a class method when used in a Rails model.
As you can see on the line 3 above ↑, there are 4 options that can be passed to
column: db column to store position in the list.
scope: restricts what is to be considered a list. For example,
enabled = trueSQL could be used as
scope, to limit list items to those that are enabled.
top_of_list: a number the first element of the list will have as position.
add_new_at: specifies whether new items get added to the :top or :bottom of the list.
Step 2: the problem with passing options
Options are passed as
options argument, and a hash is expected (see the line 2 above ↑). Then, the default
configuration hash is updated with the passed
options on the line 4, thus overriding defaults with the passed options.
The problem here is that the caller can make mistakes:
- By passing not a
Hash, but something else:
- By passing a wrongly spelled option (
acts_as_list columm: "order"
In both cases,
.acts_as_list will fail silently, leaving the user to figure out what went wrong by themselves.
Using Ruby 2 keyword arguments solves both described problems:
Step 3: configuration variable
configuration variable after using keyword arguments does look confusing, and it’s so much shorter without it:
I realise that it puts cognitive load on us, to figure out that
scope is part of configuration, but if the method is short (and currently it’s not short), it’ll be ok. Meanwhile, I’ll enjoy shorter names :)
Step 4: the problem with ad-hoc solution
As you can see on the line 3 above ↑,
_id suffix is added to
scope. The problem with this line is twofold:
- It tells the story of what it does, but doesn’t tell why.
- It’s an ad-hoc solution, and is harder to read than a standard solution.
I thought of extracting that into a method (thus, solving the 1st problem), but fortunately, I guessed that there must be a method out there doing that already. And indeed, there is: ActiveSupport::Inflector.foreign_key. So, I’m going to use it:
#foreign_key method fits perfectly here, because,
scope is described in the comments as Given a symbol, it’ll attach _id and use that as the foreign key restriction. Not only it’s a standard solution, the story it tells, fits well into what
As you can see on the line 2 above ↑, I’ve chosen to include
ClassMethods, thus polluting all classes
ClassMethods will be extending. But this is temporary, and I’ll figure out later, how to fix that.
Step 4.1: a hairy conditional
On the lines 5-7 (see above ↑), we add
_id suffix to
scope if it’s a
Symbol and doesn’t end with
_id already. This code is ripe for extracting a method:
On the line 2 (see above ↑) you can see that I haven’t extracted the check of whether
scope is a
Symbol. I believe, it would be less readable to have just
scope = idify(scope) as it’d look like we add
_id suffix always. But this is not the case, the suffix is added only for symbols (strings are left untouched).
However, there’s one problem with this setup. Having
#idify in the module
ClassMethods pollutes namespace of
Step 5: split .acts_as_list into smaller pieces
At this stage,
.acts_as_list method is 118 lines long. Here’s a short snippet:
The code in
.acts_as_list defines methods and Rails callbacks, related to
add_new_at arguments. It seems like a good idea to group code by those arguments, putting
scope-related stuff into one place and
column-related, into some other place.
Step 5.1: ways to split .acts_as_list
I see 3 approaches to split
.acts_as_list, and I’m going to describe them below.
To avoid polluting
ClassMethods namespace, add a module
AuxMethods and split
.acts_as_list into multiple methods. It’d look something like this:
The problem with this approach is that methods names aren’t very readable. Also, since we can’t include
ClassMethods, we can’t get rid of
AuxMethods. prefix. And it doesn’t read that well too.
Approach 2: a service object
A service object could look like this:
I think, it’s even worse than the approach 1. It looks like the methods that are defined when
#define_column_methods is called, are defined on the
definer object. And, it’s one line longer.
Approach 3: multiple modules
This is my favourite of the three, because:
- The first thing you read is what argument the defined methods belong to, e.g.
- The modules’
.callmethods only take the arguments the modules need (better than the approach 2).
Step 5.2: the result of splitting into multiple modules
After the module extraction (I chose the approach 3),
.acts_as_list looks like this:
So, instead of 118 lines,
.acts_as_list is 30 lines now, and fits into a page.
Step 5.3: redundant class_eval
Exactly because I have reduced the number of lines, I can now pay more attention to what’s left. And, on the line 9 (see above ↑) there’s a redundant
.class_eval call. This call changes execution context from
self to, well,
self. That’s why it’s redundant. After removal, we get (see the lines 9-11 below ↓):
Step 5.4: Rails callbacks
On the lines 13-25 (see above ↑), there are lots of Rails callbacks created. I’ve already added
ColumnMethodDefiner.call, etc, so having callback code here breaks Single Level of Abstraction. I’ve extracted the Rails callbacks into a separate module (see the line 13 below ↓):
Step 5.5: #acts_as_list_class method
If Rails callbacks break Single Level of Abstraction, doesn’t code on the lines 9-11 (see above ↑) break it too? It does. Because it’s so small, it seems that there’s no harm in having it there as it is, but I don’t really care to read that
#acts_as_list_class is added, I’d rather read a high-leveled description of what kind of functionality it provides.
So, I’ve looked up the rest of the code and,
#acts_as_list_class is just used internally by the gem. So, it’s an auxiliary method. I’ve extracted it into its own module (see the line 9 below ↓):
Is this the best I can do?
I could possibly treat definers as plugins and load them with:
But I think it’d be an overkill. My main argument against that is that these modules aren’t really plugins. If there was a standard way to add plugins in Ruby, that might have been plausible, but adding an ad-hoc plugin system would only make things more complicated. And instead of reading a number of
.calls, reader would have to figure out the plugin system. A no-go.
So, that’s the best I can do with this method (as per step 4.5).
What to expect from part 2?
In part 2 I’ll reap the consequences of choosing the approach 3 to split
.acts_as_list into modules, and will refactor one of those modules. I’ve already started on that, so I can say that it’s interesting to see how the choice to use a separate module allowed to further improve the code by extracting methods. Single Responsibility Principle isn’t there for nothing after all :)
If you want to know when the part 2 is out, sign up for my email list.