Refactoring a Ruby Module Mixin into a Class // Take on Rules

This past week I’ve been pairing more than usual with one of my co-worker’s. We’re working through upgrading Hyku 📖 and it’s dependencies; namely Bulkrax 📖 and Hyrax. Our pairing involves refactoring the application and dependencies away from one Object Relational Mapper (ORM 📖) pattern and into another (example given (e.g. 📖) from ActiveRecord-esque towards Data Mapper). Background In the case of Bulkrax , we’re looking at supporting both ORM implementations. In part because we know that not everyone will be able to immediately upgrade; ourselves as well as other Samvera 📖 community members. We have several clients whom are intending to move to the upgraded version of Hyku , but due to project realities that might be several months. Which means by focusing on backwards compatibility, we don’t have to worry about back-porting fixes; we can update the main branch and they can use those updates. In reviewing the code, I found a module (e.g. Bulkrax::FileFactory) that was mixed into a single class (e.g. Bulkrax::ObjectFactory), as instance variables. That module called instance methods of the class and the class’s instance called methods from the module. As part of the refactoring we were introducing another class (e.g. Bulkrax::ValkyrieObjectFactory) that was to implement the same public interface as Bulkrax::ObjectFactory. And we needed to change methods in the Bulkrax::FileFactory. Instead of adding conditionals within Bulkrax::FileFactory I chose to extract the module into a class and more clearly define the interface between an ObjectFactory and FileFactory. End State of Refactor In reviewing the code, the Bulkrax::FileFactory called the following methods from Bulkrax::ObjectFactory: And rather sneakily Bulkrax::FileFactory set the @update_files instance variable of Bulkrax::ObjectFactory. (Sidenote: I identified other issues with this mutation of an instance_variable, but chose to preserve the functionality as part of the refactor.) Looking the other direction, the Bulkrax::ObjectFactory called two methods defined in Bulkrax::FileFactory: file_attributes remove_file_sets I then set about extracting a class from the module. Below is the outline of that implementation: module Bulkrax module FileFactory extend ActiveSupport::Concern included do def file_factory_inner_workings @file_factory_inner_workings ||= Bulkrax::FileFactory::InnerWorkings.new(object_factory: self) end private :file_factory_inner_workings delegate(:file_attributes, :remove_file_sets, to: :file_factory_inner_workings) attr_writer :update_files end end ## # Rubocop won't like this but in declaring the class this way, all methods # will maintain their current indentation, and I won't be to "blame" for these # changes class FileFactory::InnerWorkings def initialize(object_factory:) @object_factory = object_factory end attr_reader :object_factory delegate(:attributes, :klass, :object, :updated_files=, to: :object_factory) # Moved all methods that were previously defined directly in # Bulkrax::FileFactory into the InnerWorkings. end end The two delegate declarations describe how these two classes collaborate. And it enforces a more clear boundary between these two concepts; something that is immediately useful as we look to create an appropriate FileFactory for the ValkyrieObjectFactory. Generalized Steps You’ll need to: And your strategies might vary based on your confidence in the test suite and its code coverage. If you have a high degree of confidence in your test suite, you can move all module methods into the new class then run the tests to see what methods need to be exposed to their corresponding collaborator. Determine Exposed Methods This step has three primary considerations: Find what includes/extends/prepends the module. Check those objects for what module methods the object privately uses. Check if any of the mixed-in methods have been called publicly; you’ll want to consider the implications of exposing this method. One strategy I used was to get all of the instance methods of Bulkrax::FileFactory. There were a few approaches to do that: In the Ruby on Rails 📖 console run the following: Bulkrax::FileFactory.instance_methods. Use a regular expression on the contents of the file: ^\s+def (\w+). I then joined those methods to create a regular expression to search the project: (method_one|method_two|method_three). The search results gave me places to look. Mercifully the methods of Bulkrax::FileFactory were unique enough names that I had a short list to review. Perform the Refactor Expose a private method that instantiates (and memoizes) the newly extracted object. Delegate to the newly extracted object all of the methods called within the original object. Delegate to the original object the methods that the module once directly called. Verify Run your test suite. Exercise your test plan. Have fellow team members review you code changes changes. Keep It Clean This is a reminder that for this refactor you want to make the least number of changes; minimize the chance that you’re the one to “git blame.” Conclusion Ruby 📖 modules are very flexible, providing a quick means of adding functionality while also providing a descriptive permeable container for that functionality. With the above approach, they can relatively easily be refactored into objects that can then encapsulate their logic. Put another way, they are a great first pass at an implementation. And that first pass may be adequate for quite some time. And when the time comes, provides some rough notes on what next to do. The refactoring approach can be applied to groups of methods within an existing class. Move those methods into a module. And let them breath for a bit. Then, if the need arises, apply the same strategy to extract an object. A few years I ago, I heard some liberating language from Gee Paw Hill. Namely the goal of refactoring should be to leave the code the same or better. Adding “the same or” to the equality gives space to explore. In the above refactor, the code should function the same. And it may not be truly necessary, but that refactor reflects my time spent thinking about the code. And the refactor captures those thoughts, commit messages and all, for future consideration.