Concerns, Callbacks, and Case Statements, Oh My!

I recently had one of the most satisfying refactorings I’ve ever had as a developer and it involved duplicating code. On purpose!

A travesty, I know, but it felt great and has proven to be a good call since making the change. The logic is easier to reason about and simpler to maintain.

I Have Concerns

This is a technical story. So you need some context to understand. This code lives in a multi-tenant application that helps organizations model and evaluate certain aspects of their operations. So we have defaults that everyone starts with and then ways for specific accounts to customize the logic.

So the codebase has several different classes that existed in a hierarchy of Global -> Customer Specific. When the Global Model was updated those changes would be rolled out to the Customer Specific Models if certain conditions were met.

At first all of these Global Models were very similar and it made sense to create a Rails Concern [https://api.rubyonrails.org/classes/ActiveSupport/Concern.html]  that would handle the logic of propagating the changes from them to the Customer Specific Models. Also, because only developers were making changes to these Global Models these changes could propagate in an after_save callback to avoid having to remember to call the method every time a change was made.

This concern started out simply enough. After the Global Model was changed the concern would look at the class and wrap up all of the logic for updating the Customer Specific Models with the proper logging and make the changes.

module DefinitionPopulatable
  extend ActiveSupport::Concern

  included do
    after_save :populate_definitions

    private

    def built_definition(account)
      if self.is_a?(FooDefinition)
        account.foo_definitions.find_by(foo_definition: self)
      elsif self.is_a?(BarDefinition)
        account.bar_definitions.find_by(bar_definition: self)
      elsif self.is_a?(BazDefinition)
        account.baz_definitions.find_by(baz_definition: self)
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_record_audit_key
      if self.is_a?(FooDefinition)
        AuditLogEntry::KEY_FOO_UPDATE
      elsif self.is_a?(BarDefinition)
        AuditLogEntry::KEY_BAR_UPDATE
      elsif self.is_a?(BazDefinition)
        AuditLogEntry::KEY_BAZ_UPDATE
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def update_definition(account)
      account_definition = built_definition(account)

      AuditLog.start(account_definition, definition_record_audit_key(account_definition)) do |entry|
        account_definition.update_from_definition(self)
        entry.log!
      end
    end

    def populate_definitions
      accounts_to_populate.find_each do |account|
        update_definition(account)
      end
    end
  end
end

That may feel like a lot at first, but if you’re familiar with the problem space, it’s simple and digestible. Just figure out what type of definition this is and propagate the updates, within an auditing context.

Over time, there was a need to either update the Customer Specific Models or create them if they did not already exist for a specific customer. This doubled the amount of paths that the concern could take.

module DefinitionPopulatable
  extend ActiveSupport::Concern

  included do
    after_save :populate_definitions

    private

    def built_definition(account)
      if self.is_a?(FooDefinition)
        account.foo_definitions.find_by(foo_definition: self).first_or_initialize
      elsif self.is_a?(BarDefinition)
        account.bar_definitions.find_by(bar_definition: self).first_or_initialize
      elsif self.is_a?(BazDefinition)
        account.baz_definitions.find_by(baz_definition: self).first_or_initialize
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_record_new_audit_key
      if self.is_a?(FooDefinition)
        AuditLogEntry::KEY_FOO_CREATE
      elsif self.is_a?(BarDefinition)
        AuditLogEntry::KEY_BAR_CREATE
      elsif self.is_a?(BazDefinition)
        AuditLogEntry::KEY_BAZ_CREATE
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_record_existing_audit_key
      if self.is_a?(FooDefinition)
        AuditLogEntry::KEY_FOO_UPDATE
      elsif self.is_a?(BarDefinition)
        AuditLogEntry::KEY_BAR_UPDATE
      elsif self.is_a?(BazDefinition)
        AuditLogEntry::KEY_BAZ_UPDATE
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_audit_key(definition)
      definition.new_record? ? definition_record_new_audit_key : definition_record_existing_audit_key
    end

    def update_definition(account)
      account_definition = built_definition(account)

      AuditLog.start(account_definition, definition_audit_key(account_definition)) do |entry|
        account_definition.update_from_definition(self)
        account_definition.save!
        entry.log!
      end
    end

    def populate_definitions
      accounts_to_populate.find_each do |account|
        update_definition(account)
      end
    end
  end
end

That’s still not too crazy bad, but it’s starting to feel like a lot. The ratio of control flow vs meaningful operations is starting to get out of whack.

Then came the concept of versioning handled through new Configuration models associated with the existing Definition models the Customer Specific Models based on when they were introduced and some flags in the Customer’s Account.

module DefinitionPopulatable
  extend ActiveSupport::Concern

  included do
    after_save :populate_definitions

    private

    def built_definition(account)
      if self.is_a?(FooConfiguration)
        account.foo_definitions.find_by(foo_definition: self).first_or_initialize
      if self.is_a?(FooDefinition)
        account.foo_definitions.find_by(foo_definition: self)
      elsif self.is_a?(BarConfiguration)
        account.bar_definitions.find_by(bar_definition: self).first_or_initialize
      elsif self.is_a?(BarDefinition)
        account.bar_definitions.find_by(bar_definition: self)
      elsif self.is_a?(BazConfiguration)
        account.baz_definitions.find_by(baz_definition: self).first_or_initialize
      elsif self.is_a?(BazDefinition)
        account.baz_definitions.find_by(baz_definition: self)
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_record_new_audit_key
      if self.is_a?(FooDefinition) || self.is_a?(FooConfiguration)
        AuditLogEntry::KEY_FOO_CREATE
      elsif self.is_a?(BarDefinition) || self.is_a?(BarConfiguration)
        AuditLogEntry::KEY_BAR_CREATE
      elsif self.is_a?(BazDefinition) || self.is_a?(BazConfiguration)
        AuditLogEntry::KEY_BAZ_CREATE
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_record_existing_audit_key
      if self.is_a?(FooDefinition) || self.is_a?(FooConfiguration)
        AuditLogEntry::KEY_FOO_UPDATE
      elsif self.is_a?(BarDefinition) || self.is_a?(BarConfiguration)
        AuditLogEntry::KEY_BAR_UPDATE
      elsif self.is_a?(BazDefinition) || self.is_a?(BazConfiguration)
        AuditLogEntry::KEY_BAZ_UPDATE
      else
        fail 'Invalid model to populate account definitions from'
      end
    end

    def definition_audit_key(definition)
      definition.new_record? ? definition_record_new_audit_key : definition_record_existing_audit_key
    end

    def upsert_definition(account)
      account_definition = built_definition(account)
      AuditLogEntry.start(account_definition, definition_audit_key(account_definition)) do |entry|
        account_definition = account_definition.initialize_from_configuration(account, self)
        entity_definition.save!
        entry.log!
      end
    end

    def update_definition(account)
      account_definition = built_definition(account)

      AuditLog.start(account_definition, definition_audit_key(account_definition)) do |entry|
        account_definition.update_from_definition(self)
        account_definition.save!
        entry.log!
      end
    end

    def populate_definitions
      return if configuration? && !latest_version?
      accounts_to_populate.find_each do |account|
        upsert_definition(account) if configuration?
        update_definition(account) if definition?
      end
    end

    def configuration?
      self.is_a?(FooConfiguration) || self.is_a?(BarConfiguration) || self.is_a?(BazConfiguration)
    end

    def definition?
      self.is_a?(FooDefinition) || self.is_a?(BarDefinition) || self.is_a?(BazDefinition)
    end
  end
end

The concern was modified to handle these new requirements, still using the same after_save callback. This all happened over the course of years, and each change made sense at the time.

But looking at in total, there’s clearly too much accidental complexity, too many conditionals, too much control flow to wade through to find the meaningful logic. The executed codepath of any given call was only a handful of lines, but dozens of lines had to be evaluated to reach it. At this point the Concern was turning into one of those places in the application where developers feared to tread.

Here Be Dragons

It was causing issues with debugging any of the logic that decided whether an update or upsert should be performed.

The nature of all of this happening in an after_save callback was causing issues when writing and running tests that needed to create or modify the Global Models.

The logic had become so complicated that making a change to the concern would involve spending a good chunk of time just trying to parse the code enough to build up a working understanding of what was happening through all of the paths to update the logic.

Up until this point these Global Models were created and updated solely by developers. There came a feature request to allow administrators for the application to be able to create and modify these models through an Admin interface in the UI. After some discussion it was decided that all of the customers using the application would be using the latest version of all of these models, which simplified the concern a little bit by removing the need to keep track of the versioning.

However, all of the changes were still being made in an after_save callback for each of these models. When an administrator made changes through the UI those changes were rolled out after the model was saved. An accidental click of the save button could result in all of the Customer Specific models having valid but incomplete data.

Something had to change.

More or Less

This was when I decided to roll up my sleeves and fix it. But the question is whether it made sense to layer on more complexity or try to remove complexity. I could have leaned into duck typing and delegation patterns, maybe try to encapsulate patterns into actor objects, etc. More sophisticated complexity could unquestionably make this concern look cleaner. But would it actually make it easier to understand the business logic?

At that point, I asked myself an important question: what is this complexity buying us?

The answer seemed to be that it was saving us from duplicating a few simple lines of code. So, I considered what it would look like if we just leaned into intentional duplication here. I started splitting out the logic from the Concern into the Global Models themselves.

class FooDefinition
  def populate_accounts
    audit_key = new_record? ? AuditLogEntry::KEY_FOO_CREATE : AuditLogEntry::KEY_FOO_UPDATE

    accounts_to_populate.each do |account|
      account_definition = account.foo_definitions.find_by(foo_definition: self).first_or_initialize

      AuditLog.start(account_definition, audit_key) do |entry|
        account_definition.update_from_definition(self)
        account_definition.save!
        entry.log!
      end
    end
  end
end

Whoa, look how simple and clear the meaningful logic is when pulled out. Even though I felt like I was duplicating existing logic by going through this exercise at first, just watching the concern grow smaller and less complicated filled me with joy. As I took more and more pieces out and put them into their new homes I could feel the mental overhead of understanding what was happening becoming more and more manageable.

The little pieces of logic that were all tangled up in case statements that varied by class became clear when placed in a method that was on the proper model. Now that everything was being moved closer to where it felt like it belonged it all became so much more manageable.

Then I removed the after_save callbacks and moved to using method calls in the controller actions of the new Admin UI. This gave us the flexibility to call the propagation of the changes explicitly when they had been saved and reviewed.

Don’t Repeat Yourself, Usually

Don’t Repeat Yourself (DRY) is one of the fundamental tenets of good software design. Unthinking duplication leads to maintenance headaches and bugs. I still believe that removing duplication is a good default position. But the longer I work in this field, the more I understand there are times that breaking the rules can improve a system.

Learn the rules so you know how to break them properly.

Tenzin Gyatso, the 14th Dalai Lama

Duplicating code here unquestionably made the system simpler and easier to understand.No longer would these changes always happen when a model was updated. No longer would anyone need to pore through all of the winding paths of the concern to figure out when a Customer Specific Model would or would not be updated or created.

Even though there was now duplication of code and logic, the implementation was better, and it felt wonderful.

Cover image is Ortelius’s 1570 Theatrum Orbis Terrarum map. Photograph from the United States Library of Congress