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.
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
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.