Skip to content

Instantly share code, notes, and snippets.

@dcuadraq
Last active November 10, 2023 02:39
Show Gist options
  • Star 8 You must be signed in to star a gist
  • Fork 2 You must be signed in to fork a gist
  • Save dcuadraq/f63476b3048c5b76318abd28831a00d6 to your computer and use it in GitHub Desktop.
Save dcuadraq/f63476b3048c5b76318abd28831a00d6 to your computer and use it in GitHub Desktop.
Rails Antipatterns: Best practice Ruby on Rails Refactoring notes

Chapter 1. Models

The Model layer should also contain the business logic of your application.

ANTIPATTERN: VOYEURISTIC MODELS

Class: A class defines the characteristics of an object, including the details of what it is (its attributes) and what it can do (its methods).

Method: A method exists on a class and defines what the class can do.

Encapsulation: Ideally, the code for a class should be relatively self-contained through encapsulation, which is the concealment of functional details of a class from the other objects that call its methods. This is typically done by limiting the methods other objects are allowed to call and exposing a public interface through which an object is exposed to the world. In Ruby, this is done with the public, protected, and private keywords.

Model: Are the classes that make up a program and the classes that will be persisted to the program’s database layer.

Solution: Follow the Law of Demeter

Ruby on Rails allows you to easily navigate between the relationships of objects and therefore makes it easy to dive deep within and across related objects. While this is really powerful, there are a few reasons it’s not ideal.

@invoice.customer.name
@invoice.customer.address.street
@invoice.customer.address.city
@invoice.customer.address.state
@invoice.customer.address.zip_code

For proper encapsulation, the invoice should not reach across the customer object to the street attribute of the address object. Because if, for example, in the future your application were to change so that a customer has both a billing address and a shipping address, every place in your code that reached across these objects to retrieve the street would break and would need to change.

The Law of Demeter, also known as the Principle of Least Knowledge, lays out the concept that an object can call methods on a related object but that it should not reach through that object to call a method on a third related object. In Rails, this could be summed up as “use only one dot.”

RoR provides a shortcut for indicating that one or more methods that will be created on your object are actually provided by a related object. The method delegate.

class Address < ActiveRecord::Base
  belongs_to :customer
end

class Customer < ActiveRecord::Base
  has_one :address
  has_many :invoices

  delegate :street, :city, :state, :zip_code, to: :address
end

class Invoice < ActiveRecord::Base
  belongs_to :customer

  delegate :name,
           :street,
           :city,
           :state,
           :zip_code,
           to: :customer,
           prefix: true
end

So you will access them like

@invoice.customer_name
@invoice.customer_street
@invoice.customer_city
@invoice.customer_state
@invoice.customer_zip_code

Solution: Push All find() Calls into Finders on the Model

Any place where you directly call finders on a model, other than inside the model itself, is a place that decreases readability and maintainability.

For example, if you wanted to create a web page that displays all the users ordered by last name.

<html>
  <body>
    <ul>
      <% User.find(order: 'last_name').each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li>
      <% end %>
    </ul>
  </body>
</html>

At the very least, including the actual logic for what users will be displayed on this page is a violation of MVC. At worst, this logic can be duplicated many times throughout the application, causing very real maintainability issues.

The controller is a better place for this logic.

class UsersController < ApplicationController
  def index
    @users = User.order('last_name')
  end
end
<html>
  <body>
    <ul>
      <% @users.each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li>
      <% end %>
    </ul>
  </body>
</html>

But you could/should go further and move it to the model itself.

class User < ActiveRecord::Base
  scope :ordered, order('last_name')
end

Solution: Keep Finders on Their Own Model

Considering this code:

class UsersController < ApplicationController
  def index
    @user = User.find(params[:id])
    @recent_active_membership = @user.find_recent_active_memberships
  end
end

class User < ActiveRecord::Base
  has_many :memberships

  def find_recent_active_memberships
    memberships.where(active: true).
                limit(5).
                order('last_active_on DESC')
  end
end

The User model now knows far too much about the Membership model’s implementation, which is a clue that you still haven’t pushed the methods far enough.

class User < ActiveRecord::Base
  has_many :memberships

  def find_recent_active_memberships
    memberships.find_recently_active
  end
end

class Membership < ActiveRecord::Base
  belongs_to :user

  def self.find_recently_active
    where(active: true).limit(5).order('last_active_on DESC')
  end
end

The application now honors the MVC boundaries and delegates domain model responsibilities cleanly. Using scopes will be:

class User < ActiveRecord::Base
  has_many :memberships

  def find_recent_active_memberships
    memberships.only_actives.order_by_activity.limit(5)
  end
end

class Membership < ActiveRecord::Base
  belongs_to :user

  scope :only_active, where(active: true) # Rails 3 syntax
  scope :order_by_activity, order('last_active_on DESC')
end

The downsides includes the abuse of the Law of Demeter among others.

ANTIPATTERN: FAT MODELS

Complexity is the number-one killer of projects today. This chapter deals with simplicity from a unique angle: simplifying the internals of a model by moving that complexity to new units in the application—modules and classes.

class Order < ActiveRecord::Base
  def self.find_purchased
    # ...
  end

  def self.find_waiting_for_review
    # ...
  end

  def self.find_waiting_for_sign_off
    # ...
  end

  def self.advanced_search(fields, options = {})
    # ...
  end

  def self.simple_search(terms)
    # ...
  end

  def to_xml
    # ...
  end

  def to_json
    # ...
  end

  def to_csv
    # ...
  end

  def to_pdf
    # ...
  end
end

Solution: Delegate Responsibility to New Classes

Ruby modules can be used for separeting out related functionality. The convertion methods :to_xml, :to_json, :to_csv, :to_pdf aren't really part of an Order object mandate. An Order object should be responsible for order-like processes: calculating price, managing line items, and so on.

Keepin' It Classy

When code doesn’t belong in the class that contains it, you should refactor the code into a class of its own. This is an application of the Single Responsibility Principle (SRP), which Uncle Bob summarized as: “There should never be more than one reason for a class to change.”

Having classes take on more than a single axis of responsibility leads to code that is brittle in the face of changing requirements.

On this case SRP can be applied by splitting those conversions methods into an OrderConvereter class:

# app/models/order.rb
class Order < ActiveRecord::Base
  def converter
    OrderConverter.new(self)
  end
end

# app/models/order_converter.rb
class OrderConverter
  attr_reader :order
  def initialize(order)
    @order = order
  end

  def to_xml
    # ...
  end

  def to_json
    # ...
  end

  def to_csv
    # ...
  end

  def to_pdf
    # ...
  end
end

Exporting the PDF version of an order is now just a matter of calling: @order.converted.to_pdf.
In object-oriented circles, this is known as composition. The Order object is composed of an OrderConverter object and any other objects it needs. The Rails association methods (for example, has_one, has_many, belongs_to) all create this sort of composition automatically for database-backed models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment