Comments and Extracting Rails Features

DHH has drawn back the curtain on how Basecamp writes software in a video series, tentatively titled 'On Writing Software (well?)', I find it highly instructive and valuable to talk about software design using real world software with alls its trade-offs, necessary messiness and complexity so neatly omitted in your standard textbook toy examples.

While I do lay out the contents of each episode, this is not a series plain transcript, but rather a way for me to engage with the challenges raised in DHHs examples, add my own thoughts and, at times, contrast his approach with the one we took for speqtor.com sharing examples from our code base.

Episode 1

While Code comments are sometimes necessary to explain certain decisions or trade-offs that aren't obvious from the code, more often than not comments are a kind of code smell.

You should ask yourself why am I writing this comment? How could the code itself be clearer and not need this comment?

Every developer is familiar with arcane, outdated comments in the midst of seemingly unrelated code, because the related code had been deleted. Another advantage of self-explanatory code apart from just being clearer (by definition) is that it preempts the problem of code and its explanation getting out of sync.

def remove_inaccessible_records
  # 30s of forgiveness in case of accidental removal
  unless person.destroyed? || bucket.destroyed
    Person::RemoveInaccessibleRecordJob.set(wait: 30.seconds).perform_later(person, bucket)
  end
end

The Basecamp codebase includes a method to remove all inaccessible records after a user has been deleting, because restoring a user's objects in the bucket is cumbersome a 30 second grace period was added in case a user is accidentally removed.

A comment explains not the control flow, but the configuration of the job.

We could simply add an explanatory variable elucidating the magic value of 30 seconds and hinting at its purpose.

def remove_inaccessible_records
  grace_period_removing_inaccessible_records = 30.seconds

  unless person.destroyed? || bucket.destroyed?
    Person::RemoveInaccessibleRecordJob.set(wait: 30.seconds).perform_later(person, bucket)
  end
end

However, the value does not vary, so why store it in a variable, it should be a constant. But instead of defining it at the top of the file, as we idiomatically would for public constants in ruby, we should prefer colocating related code and making the constant private.

private
GRACE_PERIOD_REMOVING_INACCESSIBLE_RECORDS = 30.seconds

def remove_inaccessible_records
  unless person.destroyed? || bucket.destroyed?
  Person::RemoveInaccessibleRecordJob.set(
    wait: GRACE_PERIOD_REMOVING_INACCESSIBLE_RECORDS
  ).perform_later(person, bucket)
  end
end

I would go a step further and separate configuration from my app code, especially since often you might want to have different values for different environments, for instance in testing environment you might want the job to execute immediately and not wait 30 seconds.

More importantly, I have a central place to go looking for configuration options in my applications and they're not scattered across my source code. In Speqtor, for example, we only send out a notification if no new notifications for a user were scheduled within a certain cool down period, so as not to clog up their inbox.

The config options are defined in config/notifications.yml

production:
  cool_off_period_in_minutes: 20
development:
  cool_off_period_in_minutes: 0.2
test:
  cool_off_period_in_minutes: 0

and included it in application.rb  under the rails namespace config.x for custom configuration.

config.x.notification = config_for(:notification)

Back to DHH, who show us an example of how some of his refactorings lead to new features in Rails. In Basecamp there is a join model for granting users administrative access to certain resource and a helper method grant that accepts a person argument and creates an entry for the person in the join model, if an entry already exists it simply returns the person record.

What might jump out at you about this method is that it commits the sin of using an exception for controlling flow. The dual offense of using framework exceptions in your code is that it also mixes two different levels of abstraction, in this case the top-level ActiveRecord API and constants from the bowels of ActiveRecord.

module Account::Administered
  extend ActiveSupport::Concern

  included do
    has_many :administratorships

    def grant(person)
	  create! person: person
        rescue ActiveRecord::RecordNotUnique
        # don't worry about dupes. Treat them the same as successful creation
        where(person: person).take
      end
    end
  end
end

The reason we are avoiding ActiveRecord's find_or_initialize_by here is that we might end up with stale reads, as find_or_initialize_by  first checks whether a record with the attributes exists using a where query and returns it if it does or else creates one with those attributes.

In applications with high load this could lead to the result returned by the where clause to being outdated, in which case the create might fail, because the record has already been created in the interim. Hence, we are first attempting to create the record and if that fails because it already exists we simply return it.

So what we actually want is create_of_find_by(person: person) which encapsulate this behavior and simplifies this code to a mere delegation:

def grant(person)
  create_or_find_by(person: person)
end

And indeed, this method has made it into Rails 6 and it's arguably what find_or_create_by should have been from the beginning.

Just a note on the topic of exceptions as a flow control; in this instance, I think it is perfectly fine to do so (and in fact the Rails method does just that), because we are relying on the database's mechanism for ensuring data consistency and simply pass the exception through to the caller. We could not have performed this check without dealing with the database exception, since this is the only interface offered to our application code.