Code Can Be Context-Aware

I was considering this code today (adapted from Campfire):

# == Schema Info
#
# Table name: projects
#
#  account_id :integer(11) => not null
#  all_access :boolean     => not null, default: false
#
class Project < ApplicationRecord
  after_save_commit :grant_access_to_everyone

  private
    def grant_access_to_everyone
      accesses.grant_to(account.users) if all_access_previously_changed?(to: true)
    end
end

I like it as-is, and wouldn't change anything — in its current context. Forgetting the context for a moment, consider the method name in isolation: Project#grant_access_to_everyone.

What should happen when I call that method? Surely everyone will be given access to the project. But what does that mean? Who's everyone? I certainly hope not every user in the system! Let's rename it to #grant_access_to_account_users to better-reflect what it does.

But that seems like a dangerous method. What if I call it on accident? Ah, good, I see there's a guard clause. Let's make it #grant_access_to_account_users_if_changed_to_all_access to communicate the possibility of a no-op, in case we forget.

But now this method seems like a variant of something. A conditional on top of native behavior. It seems like #grant_access_to_account_users! should exist as well — including ! as a danger indicator that we won't be performing any checks and this should be ran with care (e.g. for a support request). We're left with something like this:

class Project < ApplicationRecord
  after_save_commit :grant_access_to_account_users_if_changed_to_all_access

  # Public because we'd call this directly in support requests
  def grant_access_to_account_users!
    accesses.grant_to account.users
  end

  private
    def grant_access_to_account_users_if_changed_to_all_access
      grant_access_to_account_users! if all_access_previously_changed?(to: true)
    end
end

There was a point in my career where I would've preferred this code. It's explicit and thus friendlier to beginners. It also does exactly as it says without requiring any interpretation. But it's forgotten all its context — and it's worse off because of it.

If only it were aware that it's only ever called in a callback. And if only it was smart enough to know that we'd never forget to scope to a single account — and write a test. The code could then live its best life. A simpler, pithy life.

It can!

Code can be context-aware — because programmers can (and should be) context-aware.

Consider the behavior we're programming. It's unlikely to evolve. And its surface area is small — it's a single method that lives side-by-side with its sole invocator (i.e. after_save_commit). Trust yourself and your team to refactor if the surface area or complexity increases, and use tests as a safety net. The pithiness of your codebase will compound and be much simpler as a result.

Let's go a bit further before wrapping up. I see a code smell. A method plus a conditional called from an ActiveRecord callback can be refactored by just calling the callback conditionally:

class Project < ApplicationRecord
  after_save_commit :grant_access_to_account_users!, if: :grant_access_to_account_users?

  private
    def grant_access_to_account_users!
      accesses.grant_to account.users
    end

    def grant_access_to_account_users?
      all_access_previously_changed? to: true
    end
end

This code is still worse than what we started with. Again, it forgot why it's there in the first place!

This is application code. Not framework code. At a glance, we know why, when, how, and by whom this is called. #grant_access_to_account_users? is over-explaining what we already know from the context of working on our app. And #grant_access_to_account_users! is unnecessarily effective. We don't have a real use for that behavior. But its existence misleads readers into thinking that we do! If they want to refactor, they'll waste time trying to find where we use that behavior to no avail.

I would argue it'd be better for the method to be less effective at what it does. So that if someone is ever tempted to reuse it, they'll need to fully understand the context before refactoring. And I trust they'll take the time to do that. But even if they don't — there's tests in place to verify.

Here's the message I'd like to keep in mind (and the reason I'm writing this post): writing as if you know nothing about the codebase will complicate your code.

If the context is complex, you can account for that as needed. But know when it's not. And trust your future self (and team) to know the difference.

The use case from above is not complex.