In this post I'm continuing refactoring of acts_as_list gem I started in part 1.
As you might remember, I've split
.acts_as_list method into several modules, each module dedicated to an option passed to the method. E.g.
ColumnMethodDefiner module defines methods related to the
column option (the option defines column name for storing record's list position).
This post is dedicated to refactoring of the
Improving ColumnMethodDefiner module
So, I've extracted code related to
column option of
ColumnMethodDefiner. Here's an excerpt:
Step 1: what is "column"?
Line 7 (see above ↑) references
column, but what column is that? Line 6 hints that we're talking about position column, i.e.
column means "name of the column that holds record's position in the list". I.e.
position_column_name. Unfortunately, it's too hard to read, so I opted for
position_column, which is easier to read:
I like that the method defined on the line 6 (see above ↑) has the same name as
#position_column method. Earlier, we had to reason as to why
column argument and
#position_column method contained the same data, were named differently. But no more! One concept less!
Step 2: inconsistent module name
At this point,
ColumnMethodDefiner module's mission is to define methods related to
position_column, but the module is named as if it works with just
Column. It is inconsistent, so I'm going to rename it to
On the line 4 (see above ↑), we still use
column argument though, but from the module name, we can infer that we talk about position column.
I would have liked to deprecate the
column argument and introduce
position_column to replace it, but that would be changing functionality, and refactoring is all about restructuring code and keeping functionality intact.
Step 3: a method that's too long
PositionColumnMethodDefiner.call is 46 lines long and starts with defining some instance methods:
Since the method is too long, I'm going to extract
Because in part 1 I've chosen to extract stuff related to position column to a separate module, I can now extract methods from
.call method and not be afraid to pollute the namespace (as opposed to a single module for all
A sidenote on what not to do
An interesting thing to note is that line 3 (see above ↑) doesn't need to be inside
.class_eval block that starts on line 5. At first, I made a mistake of putting the
.define_instance_methods method call inside the block, and it led to a problem. The problem was that inside
self points not to the
PositionColumnMethodDefiner module, and I had to do a hack to call
.define_instance_methods. It was ugly! Feast your eyes on this:
Step 3.1: extract class method definitions
Starting at the line 12 (see below ↓), there are several class methods defined via
I'm going to extract those class method definitions into a method:
Sidenote about Object#define_singleton_method
It was my first time encountering
#define_singleton_method, and the docs didn't explain it well: "Defines a singleton method in the receiver". WTF is a singleton method? I know the singleton pattern, but that doesn't make any sense here.
It turns out, a singleton method is a method defined on an object instance. A class, for example,
Object class, is an instance of class
Class, so a class method
Object.foo) is a singleton method too. As well as a method defined on a string:
s = "abc"
So, in Ruby
def self.foo method is a class method, and at the same time, a singleton method.
If you feel like diving into this a bit more, there's a great article Ways to Define Singleton Methods in Ruby.
Step 4: mass assignment protection
After I've extracted class and instance method definition we're left with adding
position_column as an accessible attribute on line 10 (see below ↓).
attr_accessible allows to specify a white list of model attributes that can be set via mass-assignment.
Step 4.1: redundand interpolation
At the line 10 (see above ↑),
position_column is interpolated and then converted to a
Symbol. We can do away with the interpolation here (see the line 10 below ↓):
Step 4.2: comments
One of the worst things you can find in code is comments, and I hate them with passion. Sometimes you can't help but have comments, sometimes it's a necessary evil, but not in this case. On the lines 7-8 (see above ↑) the comments explain that we only protect
position_column from mass-assignment if the user already uses mass-assignment protection. Can we say the same thing without comments? Absolutely!
So, instead of a long conditional, we have a method call
.mass_assignment_protection_was_used_by_user?, that is much easier to understand and is at the right level of abstraction.
However, lines 7-9 (see above ↑) are still at the wrong level of abstraction, so I'm going to extract them into a method:
So, I've extracted protecting
position_column attribute into
.protect_attributes_from_mass_assignment method (see line 7 above ↑).
I feel it reads much better without any comments now.
Step 4.3: .mass_assignment_protection_was_used_by_user?
Let's see whether the code that I've extracted can be improved:
On the line 3 (see above ↑) we check whether
accessible_attributes is defined. But what is
accessible_attributes? It seems that it's an undocumented Rails method.
In Rails 2.3.8
accessible_attributes used to reference
attr_accessible attribute (used to store those attributes that allow mass assignment). In Rails 4,
attr_accessible was removed in favour of strong parameters and thus, would no longer be defined.
This explains why
accessible_attributes may not be defined, and I will not dive deeper into undocumented Rails stuff.
Step 4.3.1: gratuitous use of defined?
defined?(accessible_attributes) returns a truthful value if
. accessible_attributes is defined. However, it would also return a truthful value if a variable named
accessible_attributes was defined. It's not very likely that such variable would be defined, but for somebody reading it thoroughly, it makes code harder to understand. "Did the author really mean that
accessible_attributes variable counts as mass protection defined?". Thus, it's better to replace
In this way, it's clear that we're looking for a method
.accessible_attributes, and there are no further questions.
Step 4.3.2: gratuitous negation
But we're not done with the
.mass_assignment_protection_was_used_by_user? method yet. On the line 3 (see above ↑) we check whether
accessible_attributes is not
#blank?. It's probably always better to avoid using negation. In this case, we can use
Now I'm happy with the method.
Step 5: too much of passing caller_class around
To remind you what the state of
.call method is:
We are passing
caller_class to each method call here. We could define a class instance variable and reference it in class methods later:
Voila! Reads much better!
Step 6: but it's not thread safe!
But alas, using a class instance variable is not thread safe :(
I have two choices here:
- Use a service object.
- Use a thread variable.
Step 6.1: using a service object
Long story short, I've refactored to this:
And, I can't stand it. The cure is worse than the disease. In the
#call method (see the lines 12-17 above ↑) I'm passing an instance variable
@position_column as a method argument. It's awful, but it's that or I have to say something like
position_column = @ position_column for the variable to be picked up by a
#class_eval block. Neither of the options are good. So, it's a no-go.
Step 6.2: using a thread variable
So, I've refactored to use a thread variable:
Much better than service object, but the cognitive load is there. It's just far from being standard to say
self.caller_class = caller_class. And thread variable instead of just another method argument? That takes much more thinking. "Why a thread variable?", "What does
self.caller_class = caller_class assignment mean?". It's a no-go either.
Step 7: back to the functional solution
So, in the end I was unable to improve on this:
Can you think of a way to improve it?
What to expect from part 3?
In part 3 I'll dive into methods defined with
.define_class_methods. Some of them use class instance variables, so they may not be thread safe. I'm looking forward to finding out.
That's all for today, and, happy hacking!
P.S. my PR was accepted by acts_as_list project!