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 #move_to_bottom and #move_higher.

Step 1: .acts_as_list introduction

.acts_as_list is available as a class method in ActiveRecord::Base when acts_as_list gem is loaded. Here's an excerpt from .acts_as_list definition:

Using 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 .acts_as_list:

• column: db column to store position in the list.
• scope: restricts what is to be considered a list. For example, enabled = true SQL 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.

Fed up working on bad code? Here's a way out!

For people that that want to stop suffering from bad code I’ve made a FREE course

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 (:columm instead of :column):

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

Using 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:

1. It tells the story of what it does, but doesn't tell why.
2. 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:

The #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 .acts_as_list does.

As you can see on the line 2 above ↑, I've chosen to include ActiveSupport::Inflector into 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 ActiveRecord::Base.

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 column, scope, top_of_list, 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.

Approach 1: a module with methods

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 AuxMethods to 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. ColumnMethodDefiner.
• The modules' .call methods 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.

Happy hacking!

Fed up working on bad code? Here's a way out!

For people that that want to stop suffering from bad code I’ve made a FREE course