Rails Pitfalls #1

With experience, Rails developers gain super-power: they can translate business requirements into Rails features automatically. In this lesson we'll slow down and do the counter-wise: take a look at the decision-making process in slow-motion.

The Feature

Let's say you were told to implement the following form:

1. Email confirmation 1️⃣ is needed to reduce the risk of getting typos in email addresses
2. We can't work with people if they don't accept our terms of services 2️⃣
3. We need addresses 3️⃣ to show a map on their user profiles, says your boss
4. No password for now. It'll be autogenerated later probably

As an experienced developer you come up with this code almost instantly:

class User < ApplicationRecord
  validates_presence_of :username, :email, :address
  validates_uniqueness_of :username, :email
  validates_confirmation_of :email   ⇠1️⃣
  validates_acceptance_of :terms_of_service   ⇠2️⃣

  geocoded_by :address   ⇠3️⃣
  before_save :geocode   ⇠3️⃣
end

Pivot #1

Your boss, of course, impressed by the speed of development, brings the new set of requirements:

1. Give our users an ability to edit their profiles
2. Downcase usernames to prevent duplicates
3. Also please set their default profile color to dark salmon
4. Moderators should be able to create and edit users, but they should provide two additional fields: profession and workplace

It takes a bit longer, but still, the changes are somewhat trivial:

• You simply add a virtual attribute to distinguish moderation mode 1️⃣,
• add validation for that moderation mode 2️⃣,
• make sure email and terms of services validations only trigger during object creation 3️⃣,
• add callbacks for the username and default profile color 4️⃣

class User < ApplicationRecord
  DEFAULT_COLOR = "#fcaa84"
  attr_accessor :moderation_mode    ⇠1️⃣

  validates_presence_of :username, :email, :address
  validates_uniqueness_of :username, :email
  validates_confirmation_of :email, on: :create                 ⇠3️⃣
  validates_acceptance_of :terms_of_service, on: :create        ⇠3️⃣
  validates_presence_of :profession, :workplace if: :in_moderation_mode?   ⇠2️⃣

  geocoded_by :address
  before_save :geocode, on: :create

  before_validation :strip_and_downcase_username, on: :create   ⇠4️⃣
  before_validation :set_default_color_theme, on: :create       ⇠4️⃣

  def in_moderation_mode?   ⇠2️⃣
    !!moderation_mode
  end

  def strip_and_downcase_username   ⇠4️⃣
    self.username = username.strip.downcase
  end

  def set_default_color_theme       ⇠4️⃣
    self.color = DEFAULT_COLOR unless color
  end
end

Pivot #2

Aaand the boss is happy again! Of course, he needs another "little tweak" to the UI:

If it's checked, then during profile creation, we should create a default organization for that user, whatever it means.

• Alright, another virtual attribute 1️⃣ to understand whether we need to create an organization or not.
• One more callback 2️⃣, and a method to create that default organization 3️⃣.

The final code:

class User < ApplicationRecord
  DEFAULT_COLOR = "#fcaa84"

  attr_accessor :moderation_mode
  attr_accessor :create_an_organization   ⇠1️⃣

  has_one :organization

  validates_presence_of :username, :email, :address
  validates_uniqueness_of :username, :email
  validates_confirmation_of :email, on: :create
  validates_acceptance_of :terms_of_service, on: :create
  validates_presence_of :profession, :workplace, if: :in_moderation_mode?

  before_validation :strip_and_downcase_username, on: :create
  before_validation :set_default_color_theme, on: :create

  after_save :create_default_organization, if: :should_create_organization?   ⇠2️⃣

  geocoded_by :address
  before_save :geocode

  def create_default_organization   ⇠3️⃣
    Organization.create(
      address: address,
      title: "#{username}'s organization", person: self
    )
  end

  def in_moderation_mode?
    !!moderation_mode
  end

  def should_create_organization?
    !!create_an_organization
  end

  def strip_and_downcase_username
    self.username = username.strip.downcase
  end

  def set_default_color_theme
    self.color = DEFAULT_COLOR unless color
  end
end

All these changes seem to be logical and Railswayish. Yet, when I used to write a code like this, I felt a certain degree of discomfort. Unfortunately, I was never able to explain to myself what is wrong with such code.

What people think of this code

To make sure I am not the only one who feels that way, I posted this code snippet on Reddit and in few other places.

Not only I found people who felt the same, but I was also surprised by the variety of opinions I've got:

• It's ok
• So-so
• I am happy because it is clear how to refactor such code in a very short time
• Looks like a temporary stub, rather than a real model.
• Too many responsibilities. Bad model!
• Dull Rails way
• Disgust
• For a rails God model it looks pretty good so far
• Fine. I understand what goes where, and how it works. It would be much worse if there were five layers of Trailblazer with five services, six mutators, and eight form objects.
• Good code. Clear naming. It is fine to keep it like this if the code is isolated.

Not only the opinions are different, but recommendations on how to improve the code are very diverse as well.

So, service objects, right?

You might be thinking now: "Oh, come on! He is going to teach us to use service objects and form objects, and how it fixes everything". Well, I used to be advocating for that a few years ago.

But it is not that simple. Splitting things into smaller pieces usually improves it to a certain degree. But if there is no strong reasoning behind this intention to split things, then it can bring even more complexity despite the original intention.

So, no. It is not about service objects, relax. It pretends to be a few levels deeper. I'll add more on this in the end.

For now, let's return to our dilemma.

Art vs Engineering

Isn't it kind of strange that for so great framework and so mature community, we still haven't come to a consensus on the best way to implement such a simple piece of functionality?

Someone might say that it is fine to have such a diversity of opinions. Although, there's a trick. If we treat programming like engineering, then there should be not that many options.

Engineers thoroughly test multiple ways of doing a single thing, then select the optimal way, document it, and use for that specific thing in the future. Maybe optimize it a bit.

What we have now is the opposite of engineering - it is the Art. Even within the single community(Rails), there are dozens of ways of doing the same thing. Maybe the Ruby languages itself inspires for creativity, I don't know.

From the engineering perspective, it means that many of us are doing it in a very inefficient way!

What's next

We're going to fix it. Not all of it, but some things we will improve. We'll take a look at the code above from a number of different perspectives. This should help you develop an eye on small "violations" here and there. Rails tolerates them in the sake of "developers' happiness", but in the long run, the combined effect of these violations hits us so hard that happiness is no longer on our plate.

It becomes more about survival, desperation, and blaming. This is exactly the point where Rails disciples turn into Rails critics:

If you think that I'll be blaming Rails, you're wrong again. Well, I actually might a bit, but all in all, it is not about Rails. It is about the development of a proper reasoning model and understanding the true meaning of the code we write.

In the next lesson, we'll talk about specific Rails features, which mask the complexity of our code, so that we keep thinking that our code is fine when it is not. Next email should hit your inbox in a few days. Stay cool!

This was the first part of the Rails Pitfalls free mini-course. If you found it intriguing enough, consider sharing it with your co-workers - it is always better to discuss an important topic in a company, rather than dealing with it on your own.

For public sharing please use this link instead.