interaction model motivation.md

The problem

Exhibit A

class Record < ActiveRecord::Base
  validates_uniqueness_of :name
end

At some point, we realize we're performing an extra query to check uniqueness every time we validate the object.

# bugfix 1
class Record < ActiveRecord::Base
  validates_uniqueness_of :name, on :create
end

That works great until some future date where KillerFeature introduces changing the name of existing records.

# bugfix 2
class Record < ActiveRecord::Base
  validates_uniqueness_of :name, if: :name_changed?
end

This intent of this code is clear and it's more or less bug free.

Exhibit B

class Record < ActiveRecord::Base
  after_commit :update_external_service
end

It works (even if annoying to test), but at some point we realize it happens too often. Eventually:

# buxfix 1, 2, or 3, depending on our past experiences
class Record < ActiveRecord::Base
  after_commit :update_external_service, if: "service_token_changed? && service_token.present?"
end

It works, it's more or less clear, it's bug free. Eventually we realize there are corner cases where it shouldn't fire.

# buxfix 4
class Record < ActiveRecord::Base
  after_commit :update_external_service, if: "service_token_changed? && service_token.present? && complex? && logic? && !not_satisfied?"
end

Well that's not readable, so we extract to:

# buxfix 4 refactor
class Record < ActiveRecord::Base
  after_commit :update_external_service, if: update_external_service?
  
  private
  
  def update_external_service?
    # fantastic comments
    service_token_changed? 
    # explaining
    && service_token.present? 
    
    # precisely what
    && complex? 
    # is going on here
    && logic?
    # for future devs (including myself)
    && !not_satisfied?
  end
end

In my experience this is a magnet for bugs and difficult to maintain through future change. In my opinion this type of private method is problematic in that it is still trying to infer scenario from the state of the attributes.

We can turn that scenario that we want to know about into state, and manage that state as the object changes instead:

# buxfix 4 refactor refactor
class Record < ActiveRecord::Base
  attr_reader :token_registration_required?
  
  after_commit :update_external_service, if: :token_registration_required?
  
  private
  
  def external_token=(val)
    @token_registration_required = true
    super(val)
  end
  
  def some_special_callback
    ...
    if special_case # we realize we don't need to register external token
    @token_registration_required = false
    ...
  end
end

Something like this works, is clear, and doesn't resist future change as much as previous examples.

We choose to ignore the fact that there is still public API available that excites a bug with this recommended approach. We need make sure no devs use that method of setting an attribute.

We also need to ensure that future development efforts maintain the state of @token_registration_required? properly.

Whether or not these are reasonable costs is up for debate.

Even if they are, or there is some other better way of doing this, that isn't what happens. Devs don't make it to the point of a virutual state attribute like this, we tend to wind up with and then live with complex logic trying to infer some nameless scenario.

The problem

In both of these examples, we are forced to use data to infer scenario, in order to maintain correct behavior. Our class contains all the logic for all the scenarios that we will (or will never) encounter, since the data could change at any point, in theory. We can extract the definition of our logic into concerns to try to make them easier to digest, but logically we are still working with a single class that statically handles all scenarios.

However, when there is a bug or a new feature, we are usually thinking about specific scenarios, and use debugging tools and hard thinking to deduce what states the data could be in, determine how that state occurred, and finally account for, correct, or prevent that state.