Summary: In this post, I provide some task background that was indicating a helpful refactor of code. I then provide the end state of what it looks like, followed by generalized steps and a conclusion; namely that refactors need not make better the code, just not make them worse. And can be used to describe and learn about the codebase.

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

• attributes
• object
• klass

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:

• Determine Exposed Methods
• Perform the Refactor
• Verify
• Keep it Clean

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.

Reply by Email

Summary: An Emacs function that wraps either a region or a function by leveraging the built-in treesitter functions.

One of my “quality of life” improvements in my text editor is related to Rubocop 📖 ; a style guide enforcer that raises issues about complexity, formatting, and other things.

When I’m coding and feel I need to ignore Rubocop ’s opinion, I will tell Rubocop to ignore the violation. I do this by adding a “disable” comment before the violation and an “enable” comment after the violation.

I wrote an Emacs 📖 function that leverages Tree Sitter 📖 to insert the comments. Note: The jf/rubocop/list-all-cops is a constant I’ve defined that lists the numerous checks

  (defun jf/treesit/wrap-rubocop (&optional given-cops)
    "Wrap the current ruby region by disabling/enabling the GIVEN-COPS."
    (interactive)
    (if (derived-mode-p 'ruby-ts-mode 'ruby-mode)
      (if-let ((region (jf/treesit/derive-region-for-rubocop)))
        (let ((cops
                (or given-cops
                  (completing-read-multiple "Cops to Disable: "
                    jf/rubocop/list-all-cops nil t))))
          (save-excursion
            (goto-char (cdr region))
            (call-interactively #'crux-move-beginning-of-line)
            (let ((indentation (s-repeat (current-column) " ")))
              (goto-char (cdr region))
              (insert "\n"
                (s-join "\n"
                  (mapcar
                    (lambda (cop)
                      (concat indentation "# rubocop:enable " cop))
                    cops)))
              (goto-char (car region))
              (beginning-of-line)
              (insert
                (s-join "\n"
                  (mapcar
                    (lambda (cop)
                      (concat indentation "# rubocop:disable " cop))
                    cops))
                "\n"))))
        (user-error "Not a region nor a function"))
      (user-error "%s is not derived from a ruby mode" major-mode)))

  (defun jf/treesit/derive-region-for-rubocop ()
    "Return `cons' of begin and end positions of region."
    (cond
      ;; When given, first honor the explicit region
      ((use-region-p)
        (cons (region-beginning) (region-end)))
      ;; Then honor the current function
      ((treesit-defun-at-point)
        (cons (treesit-node-start (treesit-defun-at-point))
          (treesit-node-end (treesit-defun-at-point))))
      ;; Then fallback to attempting to find the containing
      ;; class/module.
      (t
        (when-let ((node
                     (treesit-parent-until
                       (treesit-node-at (point))
                       (lambda (n) (member (treesit-node-type n)
                                     '("class" "module"))))))
          (cons (treesit-node-start node) (treesit-node-end node))))))

I’ve bound this to C-c w r; mnemonically this is “Command Wrap Rubocop.”

A few times per week, I use the jf/treesit/wrap-rubocop function. A small improvement in my work experience.

Reply by Email

Summary: Spending an hour to write up a new completion function to help in writing blog posts and documents.

I’ve created a few different macros within Org-Mode 📖 . What I wanted was to enable Completion at Point Function (CaPF 📖) for macros defined within the current buffer. (Sidenote: See Macro Replacement (The Org Manual) for documentation on Org-Mode macros. )

I had previously implemented Completion at Point Function (CAPF) for Org-Mode Links and used that for guidance.

I broke this down into three conceptual functions:

First, create a list of all matches for a regular expression within the buffer. Implemented via jf/regexp-matches-for-text.

(defun jf/regexp-matches-for-text (regexp &optional string)
  "Get a list of all REGEXP matches in the STRING."
  (save-match-data
    (seq-uniq
     (let ((string (or string (buffer-string)))
           (pos 0) matches)
       (while (string-match regexp string pos)
         (let ((text (match-string 0 string)))
           (set-text-properties 0 (length text) nil text)
           (push text matches))
         (setq pos (match-end 0)))
       matches))))

Second, get all of the existng macros within the current buffer. This leverages jf/regexp-matches-for-text and is implemented via jf/org-macros.

(defun jf/org-macros (&optional given-macro)
  (let ((macros (jf/regexp-matches-for-text "{{{[^}]+}}}")))
    (if given-macro
      (when (member given-macro macros) (list given-macro))
      macros)))

The nuance of providing a given-macro relates to the third function. Namely a CaPF conformant defintion. This is implemented in jf/org-capf-macros.

(defun jf/org-capf-macros ()
  "Complete macros.

And 3 shall be the magic number."
  ;; We're looking backwards for one to three '{' characters.
  (when (and (looking-back "{+" 3)
          (not (org-in-src-block-p)))
    (list
      ;; The beginning of the match is between 1 and 3 characters before
      ;; `point'.  The math works out:
      ;; `point' - 3 + number of '{' we found
      (+ (- (point) 3)
        (string-match "{+" (buffer-substring (- (point) 3) (point))))
      (point)
      ;; Call without parameters, getting all of the macros.
      (jf/org-macros)
      :exit-function
      (lambda (text _status)
        (let ((macro (car (jf/org-macros text))))
          (delete-char (- (length text)))
          (insert macro)))
      ;; Proceed with the next completion function if the returned
      ;; titles do not match. This allows the default Org capfs or
      ;; custom capfs of lower priority to run.
      :exclusive 'no)))

When I add jf/org-capf-macros to my Org-Mode CaPF and I type { then Tab, I am presented with a list of existing macros I’ve declared within the buffer. And I can select those and insert the macro definition at point.

I’ve added the above code to my Emacs configuration.

Conclusion

Given that I use Org-Mode macros for transforming text to specific inline markup, I found that when I was writing something that used a macro, I would often repeat usage of that macro.

For those curious, here are some of my macros. And here are some templates for populating those macros.

My templates for inserting a macro scour my Personal Knowledge Management (PKM 📖) for macro declarations and provide a list to select from. This is a bit slower and less specific than searching within the buffer.

Reply by Email