Refactoring a fat Rails Model

Whoever coined the ‘Skinny Controllers, Fat Models’ convention for MVC frameworks forgot to emphasize on a model’s single responsibility principle. Sure keep your controllers nice and clean but don’t do it at the expense of your model handling business logic that it’s not supposed to. In Rails, for instance, Active Record Models should only have 1 responsibility: Active Record. That’s it. It should not, for example, be responsible for sending emails to or about the object it encapsulates.

Recently I was assigned a project from a new client who had it developed from someone else; someone who was now suddenly unavailable to make some ‘final revisions’ and deploy it online. This application, simply put, is an appointment management portal. Stylists (hairdressers) add Offers and Users avail these offers to book an Appointment through a mobile app and a website. All new appointments are visible in a dashboard from where the Admin (portal administrator) follows up with the user via phone call to confirm their booking. Once approved, Stylists can see confirmed appointments in their dashboard.

With great hope that I won’t scare and lose the readers of this post, allow me to share a code snippet from the appointment model of this project (just as I received it):

Before we start refactoring it, let’s look at the model one more time with only method names (and some comments).

And here are the three things, related to this model, that the client asked us to do immediately after we got this project:

1. 1. Alert portal Admin whenever new appointment is created
   2. Alert Stylist whenever an appointment is confirmed
   3. Add new call status ‘answered and cancelled’ as during testing we discovered that sometimes people can book an appointment by mistake while using the app and then deny booking it.

Other than the APPOINTMENT_STATUS constant defined on top of the model, Notice that there are 5 distinct parts:

• • Scopes
  • Associations
  • Validations
  • Callbacks
  • Methods

Let’s look at each part one by one:

Generally speaking there is nothing wrong here, but it can be better. Personally, I would prefer to define the APPOINTMENT_STATUS constant in the constant.rb file and create 1 scope like this:

The only part in this model we didn’t have to look twice at.

The first two validations look fine, it’s the third (and last) one that bothered me:

 
validates_uniqueness_of :time, scope: [:offer_id, :date], conditions:
 -> { where.not(status: [APPOINTMENT_STATUS[2],
 APPOINTMENT_STATUS[3]]) }, if: Proc.new { |a| a.date and a.time }

After spending some time I realized that the appointment forms do not have date and time in them. In fact, they have a single datetime field that looked like this:

Then in the model the field was being split in two by a before_validation callback:

before_validation :appointment_date_timedef appointment_date_time
  if self.date_time.present?
    self.date = self.date_time.to_date
    self.time = self.date_time.to_time.strftime("%H:%M")
  end
end

So this last validation has

• • an if condition to only run when date & time was set and appointment status was not cancelled
  • additional constraints to limit uniqueness by offer_id, date and appointment status not cancelled or rescheduled

Since there are a total of four values in APPOINTMENT_STATUS, the following two conditions mean the same thing:

conditions: -> { where.not(status: [APPOINTMENT_STATUS[2],
APPOINTMENT_STATUS[3]]) }

and

conditions: -> { where(status: [APPOINTMENT_STATUS[0],
APPOINTMENT_STATUS[1]])

I prefer the latter because, in my opinion, just ‘where’ is more readable than ‘where.not’

As for the Split the datetime field in forms into separate date and time fields and remove the redundant date_time field from the table. Make the validation more readable and remove the if clause as its no longer required:

validates_uniqueness_of :time, scope: [:offer_id, :date], conditions:
 -> { where(status: [APPOINTMENT_STATUS[0], APPOINTMENT_STATUS[1]]) },
 if: Proc.new { |a| a.date and a.time }

Callbacks can be very handy, but like I have explained in another post about callbacks; they change the linear flow of your application and can make testing and debugging a pain. We have already removed one callback and its related method while refactoring the last validation check above. Lets look at the remaining callbacks:

before_create :add_initial_call_status
after_create :alert_on_appointment_creation
before_update :status_change_alert
after_create :generate_barcode
before_update :refund_if_cancelled_by_s_or_a

Five (originally Six) callbacks here I believe are too many. First two questions to ask yourself when creating a callback are:

1. 1. Does this functionality really belong here?
   2. Does is satisfy the single responsibility principle?

Let’s look at our callbacks:

before_create :add_initial_call_status

Appointment is one thing. Following up on that appointment is another. As we can already see that once this callback sets an initial call status, there is toggle_call_status method that switches between 3 different statuses (i.e. not called, no answer and called) and now the client asked us to add a fourth one, it’s obvious that Appointment and AppointmentFollowUp can be two seperate ActiveRecord models with a one to many association and we can track complete calling history with the user along with some notes about that call.

So here is our new model

class AppointmentFollowUp

  belongs_to :appointment

end

and we added this to the constants.rb file too:

APPOINTMENT_CONFIRMATION_CALL_STATUS = ['not called', 'called', 'no
 answer', 'answered and cancelled']

Now we don’t really need a ‘not called’ initial record to be created in AppointmentFollowUp, if Appointment has no follow_ups that means no one has called yet. And once someone does call, we can create a new record followed by another new record for every additional call.

after_create :alert_on_appointment_creation before_update :status_change_alert

Sending Alerts, Notifications, Emails are clearly separate and should have their own Models:

• • Service models or POROs (Plain Old Ruby Objects) if you don’t want a record of every communication sent to customer or if that record is being maintained by a third-party service like Pusher or MailChimp.
  • ActiveRecord models if you want the record and want to maintain it in your primary application database (not recommended).

So here is our new AppointmentNotification class:

Now that we have a seperate class for Appointment Notifications, we will create a background job to send this notification after appointment is created. Using ActiveJob, it will look something like this:

class AppointmentCompletionJob < ApplicationJob
  queue_as :default
  def perform(appointment_id)
    an = AppointmentNotification.new(appointment_id)
    an.appointment_created
  end
end

we will also need another job for appointment update

class AppointmentUpdateJob < ApplicationJob
  queue_as :default
  def perform(appointment_id, status)
    noti = AppointmentNotification.new(appointment_id)
    noti.appointment_status_updated(status)
  end
end

Personally I would like calling these jobs at the controller level but this application has 4 different sets of controllers (admin, stylist, web and mobile api) and they require a lot of refactoring of their own so at the moment we will stick to using callbacks:

after_create_commit :appointment_completion
after_update :appointment_status_change, if: -> { status_changed? }

def appointment_completion
  AppointmentCompletionJob.perform_later(id)
end

def appointment_status_change
  AppointmentUpdateJob.perform_later(id, status)
end

Let’s live with this for now.

after_create :generate_barcode

This generate_barcode method disturbed me the most in this class:

1. 1. It’s not asynchronous,
   2. It runs whenever a new appointment is created,
   3. It generates an image file and saves it on disk
   4. User has to wait to get a response until all this is done.

While this entire method needs to be redone, the first and most important thing is that Barcode is part of the receipt, and code already includes a separate PORO for receipt. So it made much more sense for us to just move it there for the time being, call it in Appointment Completion background job (created above) and worry more about it when we refactor that model. Here’s our updated background job:

class AppointmentCompletionJob < ApplicationJob
  queue_as :default
  def perform(appointment_id)
    noti = AppointmentNotification.new(appointment_id)
    noti.appointment_created
    receipt = AppointmentReceipt.new(appointment_id)
    receipt.generate_barcode
  end
end

and now to our last callback

before_update :refund_if_cancelled_by_s_or_a

Ok, so this is the only callback that made sense here. As I learned from this article many many years ago “Use a callback only when the logic refers to state internal to the object.”

After all the refactoring done above, these are the methods that are left untouched:

#reminders to sent to user before appointment time
def self.appointment_alerts
end

#method called by appointment_alerts method above
def self.send_user_appointment_alert(user, alert_time, alert_type)
end

#method called by send_user_appointment_alert method above
def self.send_appointment(user, appointment)
end

These are static methods used to send reminders to Users about their appointments. The first method ‘appointment_alerts’ is called by a background job and it then calls the remaining two methods. By now you must’ve already guessed it: this functionality belongs to a separate class too.

Here’s the new model for your reference which contains this functionality in better much smaller and readable methods:

This class can now be called in the background job that was originally calling Appointment.appointment_alerts method.

Here is our Appointment model now:

It’s still not perfect, but if you compare it with the original code shared in the beginning of this article, you will find this much more readable and smarter. Ideally, I would want it to look like this:

and move the logic from the two removed callbacks in separate service objects for appointment creation and appointment update (for example by using active interaction gem) and then call those new objects in each of the controller sets so that same logic is not repeated 4 times (remember 4 controller sets: admin, stylist, web and mobile api).

While working with Models in MVC:

1. 1. Always create Smart (not Fat) Models with Single Responsibility.
   2. Try to follow the Open Close Principle as much as possible (as I know it’s hard to anticipate what kind of change may come in the future; you can never make it perfect, but always try to make it as good as possible)

and in addition to above, while working with Models in Rails

1. 1. Don’t get trigger-happy with callbacks. Use them with caution.
   2. Don’t use ‘self.’ with attribute names in the model. That’s unnecessary.
   3. Ruby has a very clean syntax, please don’t write dirty code with it. Write good readable code with easy to understand variable/method names and good comments.

This article was written while the refactoring work for the entire project was in process, I’m sure there are still a lot of things that can be improved here. Your feedback is welcome and will be appreciated.

Since this is a real life project, I could only share parts and not all of code. Hope what was shared was enough for understanding of this article. If you have any questions, please feel free to ask.