My take on Refactoring Rails' initializers
Last week, I setup a refactoring challenge for a file in Rails' source code. You can read that post here.
There were no submissions from other people, so we'll just be looking at my take.
The original Rails initializable.rb
Here's the original initializable.rb
from Rails.
require "tsort"
module Rails
module Initializable
def self.included(base) # :nodoc:
base.extend ClassMethods
end
class Initializer
attr_reader :name, :block
def initialize(name, context, options, &block)
options[:group] ||= :default
@name, @context, @options, @block = name, context, options, block
end
def before
@options[:before]
end
def after
@options[:after]
end
def belongs_to?(group)
@options[:group] == group || @options[:group] == :all
end
def run(*args)
@context.instance_exec(*args, &block)
end
def bind(context)
return self if @context
Initializer.new(@name, context, @options, &block)
end
def context_class
@context.class
end
end
class Collection < Array
include TSort
alias :tsort_each_node :each
def tsort_each_child(initializer, &block)
select { |i| i.before == initializer.name || i.name == initializer.after }.each(&block)
end
def +(other)
Collection.new(to_a + other.to_a)
end
end
def run_initializers(group = :default, *args)
return if instance_variable_defined?(:@ran)
initializers.tsort_each do |initializer|
initializer.run(*args) if initializer.belongs_to?(group)
end
@ran = true
end
def initializers
@initializers ||= self.class.initializers_for(self)
end
module ClassMethods
def initializers
@initializers ||= Collection.new
end
def initializers_chain
initializers = Collection.new
ancestors.reverse_each do |klass|
next unless klass.respond_to?(:initializers)
initializers = initializers + klass.initializers
end
initializers
end
def initializers_for(binding)
Collection.new(initializers_chain.map { |i| i.bind(binding) })
end
def initializer(name, opts = {}, &blk)
raise ArgumentError, "A block must be passed when defining an initializer" unless blk
opts[:after] ||= initializers.last.name unless initializers.empty? || initializers.find { |i| i.name == opts[:before] }
initializers << Initializer.new(name, nil, opts, &blk)
end
end
end
end
What stands out about this file's implementation?
- Immediately, the
klass.extend ClassMethods
is a clue that we could be we using anActiveSupport::Concern
, since this is in Rails, to express the same idea more clearly.
Part of refactoring is restating your boilerplate or homegrown code with a more well-known/named concept.
- We've got a custom
Initializer
class yet we're still mucking around with an internal@options
hash and addingattr_reader
-like methods for those.
- We're doing an Array subclass for this relatively short file that seems fairly self-contained. There seems to be a bunch of related complexity, of packing and unpacking into this
Collection
class including the+
override, stemming from this design choice. This class is just to get access totsort_each
, so I wonder if there's another way to do that?
- The
bind
and returning a newInitializer
looks like some old-school Ruby-vernacular that I don't quite understand why we're doing this. I wonder if there's a simpler way?
My therapeutic refactoring of initializable.rb
Originally, I explored refactoring this file 2 years ago. You can see the gist here.
I think I spent 2 hours on it? I don't quite remember.
Here's what I ended up with, you can see it tries to address my concerns with the code above.
require "tsort"
module Rails
module Initializable
extend ActiveSupport::Concern
def run_initializers(group = :default, *arguments)
@ran ||= true.tap do
Initializer.tsort(initializers).each { instance_exec(*arguments, &_1.block) if _1.in?(group) }
end
end
def initializers
self.class.ancestors.reverse.flat_map { _1.initializers if _1.respond_to?(:initializers) }
end
class_methods do
def initializer(name, group: :default, before: nil, after: nil, &block)
raise ArgumentError, "A block must be passed when defining an initializer" unless block
after ||= initializers.last&.name unless initializers.any? { _1.name == before }
initializers << Initializer.new(name:, block:, group:, before:, after:)
end
def initializers = @initializers ||= []
end
class Initializer < Data.define(:name, :block, :group, :before, :after)
def initialize(name:, block:, group: :default, before: nil, after: nil) = super
def self.tsort(initializers)
TSort.tsort initializers.to_enum,
->(initializer, &block) { initializer.pluck(initializers).each(&block) }
end
def pluck(initializers)
initializers.select { _1.before == name || after == _1.name }
end
# Alternative to pluck above to try to name, but the original inner select makes no sense to me.
def around?(other) = self < other || self > other
def >(other) = other.before == name
def <(other) = after == other.name
def in?(group)
self.group == group || self.group == :all
end
end
end
end
To best understand this code and compare, I recommend retyping it manually (you'd be surprised!) or popping open a console to try play around with bits of it.
Let me know how you liked this and what stood out to you.
Do you feel more comfortable attempting a therapeutic refactoring in your app or in Rails itself?