Exploring Rails Anti-Patterns Elle Meredith [email protected] 

• Controllers • Views • Models • Requests What’s the plan doc? 

Our little shop app 

Common approaches to recurring problems that ultimately prove to be ineffective What are anti-patterns? 

Common approaches to recurring problems that ultimately prove to be ineffective What are anti-patterns? Improving the quality of code without changing behaviour What is refactoring? 

Fat controllers 

Overloading controllers with logic # app/controllers/orders_controller.rb class OrdersController < ApplicationController def placed @order = current_object.is_a?(Cart) ? current_object.convert_to_order! : current_object @order.cancel_payment_processing! @order.place! if @order.new? && @order.balances? unless @order.confirmation_sent? OrderMailer.deliver_confirmation(order_id: @order.id) @order.update_attribute(:confirmation_sent, true) end update_session! end end 

Solution: move business logic out to models, presenters, and service objects # app/services/place_order.rb class PlaceOrder attr_accessor :order def initialize(order) @order = if order.is_a?(Cart) ? convert_to_order! : order end def call if place! # send confirmation email end end private def place! if order.new? && order.balances? # save end end end 

Solution: move business logic out to models, presenters, and service objects # app/controllers/orders_controller.rb class OrdersController < ApplicationController def create placed_order = PlaceOrder.new(@order).call update_session! # handle the response end end 

Fat controllers # app/controllers/users_controller.rb class UsersController < ApplicationController def customers @users = User.all if not params[:operation].nil? then if (params[:operation] == "reset_password") then user = User.find(params[:id]) user.generate_password_reset_access_key user.password_confirmation = user.password user.email_confirmation = user.email user.save! flash[:notice] = user.first_name + " " + user.last_name + "'s password has been reset." end end end end Q: what are the problems with this code? 

Fat controllers # app/controllers/users_controller.rb class UsersController < ApplicationController def customers @users = User.all if not params[:operation].nil? then if (params[:operation] == "reset_password") then user = User.find(params[:id]) user.generate_password_reset_access_key user.password_confirmation = user.password user.email_confirmation = user.email user.save! flash[:notice] = user.first_name + " " + user.last_name + "'s password has been reset." end end end end Q: what are the problems with this code? 

Solution: embrace REST # app/controllers/users_controller.rb class UsersController < ApplicationController def index @users = User.active end end # app/controllers/password_resets_controller.rb class PasswordResetsController < ApplicationController def new # display a form to enter a new password end def create reset_password = ResetPassword.new(params[:user_id]).call # handle result end end 

Spaghetti SQL — long queries # app/controllers/orders_controller.rb class OrdersController < ApplicationController def show order = Order.where(id: params[:id], customer: current_user).first line_items = LineItem .where(order: order, name: "Computer") .or(LineItem.where(order: order, name: "Phone")) render locals: {order: order, line_items: line_items} end end 

Spaghetti SQL — long queries # app/controllers/orders_controller.rb class OrdersController < ApplicationController def show order = Order.where(id: params[:id], customer: current_user).first line_items = LineItem .where(order: order, name: "Computer") .or(LineItem.where(order: order, name: "Phone")) render locals: {order: order, line_items: line_items} end end 

Spaghetti SQL — long queries # app/views/orders/show.html.erb

Line items↙h3> <% line_items = LineItem .where(order: @order, name: "Computer") .or(LineItem.where(order: @order, name: "Phone")) %>

<%- line_items.each do |line_item| %>
• <%= line_item.name %>↙li> <%- end %> ↙ul> 

Solution: utilise ActiveRecord queries better # app/models/line_item.rb class LineItem < ApplicationRecord def self.of_interest where(name: "Computer").or(where(name: "Phone")) end end # app/controllers/orders_controller.rb class OrdersController < ApplicationController def show order = current_user.orders.where(id: params[:id]).first line_items = order.line_items.of_interest render locals: {order: order, line_items: line_items} end end 

N+1 queries def line_items_for_recent_orders orders = Order.limit(5) orders.flat_map do |order| order.line_items.group_by(&:name) end end Order Load (0.6ms) SELECT "orders".* FROM "orders" LIMIT $1 [["LIMIT", 5]] LineItem Load (0.3ms) SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1 [["order_id", "abc"]] LineItem Load (0.2ms) SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1 [["order_id", "def"]] LineItem Load (0.3ms) SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1 [["order_id", "ghi"]] LineItem Load (0.1ms) SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1 [["order_id", "jkl"]] LineItem Load (0.1ms) SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1 [["order_id", “mno"]] 

Solution: eager loading def line_items_for_recent_orders orders = Order.includes(:line_items).limit(5) orders.flat_map do |order| order.line_items.group_by(&:name) end end Order Load (0.3ms) SELECT "orders".* FROM "orders" /* loading for pp */ LIMIT $1 [["LIMIT", 5]] LineItem Load (0.6ms) SELECT "line_items".* FROM "line_items" WHERE “line_items"."order_id" IN ($1, $2, $3, $4, $5) [["order_id", “c924e315-f9d2-482e-9bf2-e230b77a5215"], ["order_id", “47576b40-916e-4a54-b4e7-e119ea81ed44"], ["order_id", “94a6a533-e64e-4d92-b1dd-b5cd6a0a096c"], ["order_id", “fc5062d4-38f3-40fd-add3-e530435b22e0"], ["order_id", "8e96d3b9-0daa-42b2-bfc9-cc9703cd2eca"]] 

Solution: eager loading def line_items_for_recent_orders orders = Order.includes(:line_items).limit(5) orders.flat_map do |order| order.line_items.group_by(&:name) end end Order Load (0.3ms) SELECT "orders".* FROM "orders" /* loading for pp */ LIMIT $1 [["LIMIT", 5]] LineItem Load (0.6ms) SELECT "line_items".* FROM "line_items" WHERE “line_items"."order_id" IN ($1, $2, $3, $4, $5) [["order_id", “c924e315-f9d2-482e-9bf2-e230b77a5215"], ["order_id", “47576b40-916e-4a54-b4e7-e119ea81ed44"], ["order_id", “94a6a533-e64e-4d92-b1dd-b5cd6a0a096c"], ["order_id", “fc5062d4-38f3-40fd-add3-e530435b22e0"], ["order_id", "8e96d3b9-0daa-42b2-bfc9-cc9703cd2eca"]] 

1. `bullet` gem 2. Rails built-in strict loading mode 3. `rack-mini-profiler` and the `stackprof`gems 4. Monitor with SaaS tools 5. Manually review your application’s log files and look for repeated queries that are being executed. How to identify N+1 queries in a Rails app? Q: Given how easy it is in Active Record to make complex queries, how do you protect against n+1? 

Knowing too much about your neighbours' neighbours class Address < ApplicationRecord belongs_to :user end class User < ApplicationRecord has_many :addresses has_many :orders def billing_address addresses.billing.first end end class Order < ApplicationRecord belongs_to( :customer, class_name: "User", foreign_key: :user_id, optional: false ) end 

Knowing too much about your neighbours' neighbours # in our view <%= @order.customer.name %> <%= @order.customer.billing_address.street %> <%= @order.customer.billing_address.city %> <%= @order.customer.billing_address.state %> <%= @order.customer.billing_address.postcode %> 

Knowing too much about your neighbours' neighbours # in our view <%= @order.customer.name %> <%= @order.customer.billing_address.street %> <%= @order.customer.billing_address.city %> <%= @order.customer.billing_address.state %> <%= @order.customer.billing_address.postcode %> Law of Demeter @order.try(:customer) .try(:billing_address) .try(:street) 

Solution 1: delegate # app/models/user.rb class User < ApplicationRecord has_many :addresses has_many :orders delegate( :street, :city, :state, :postcode, to: :billing_address, prefix: :billing ) end # app/models/order.rb class Order < ApplicationRecord belongs_to( :customer, class_name: "User", foreign_key: :user_id, optional: false ) delegate(:name, to: :customer, prefix: true) delegate( :billing_street, :billing_city, :billing_state, :billing_postcode, to: :customer ) end 

Solution 1: delegate # in our view <%= @order.customer_name %> <%= @order.billing_street %> <%= @order.billing_city %> <%= @order.billing_state %> <%= @order.billing_postcode %> Q: why is this better? 

Solution 2: a presenter with a SimpleDelegator # app/presenters/order_presenter.rb class OrderPresenter < SimpleDelegator def initialize(order) super(order) end def customer_name customer.name end def billing_street customer.street end #... end 

Solution 2: a presenter with a SimpleDelegator # app/presenters/order_presenter.rb class OrderPresenter < SimpleDelegator def initialize(order) super(order) end def customer_name customer.name end def billing_street customer.street end #... end # in our view <%= @order.customer_name %> <%= @order.billing_street %> <%= @order.billing_city %> <%= @order.billing_state %> <%= @order.billing_postcode %> 

Dealing with nils undefined method `street' for nil:NilClass @order.try(:customer) .try(:billing_address) .try(:street) 

Dealing with nils # app/views/orders/show.html.erb <%- if order.customer.billing_address.present? %>

Customer billing address: ... ↙p> <%- else %>

No address↙p> <%- end %> 

Null object pattern # app/models/null_address.rb class NullAddress def street "No street" end def city "No city" end def state "No state" end def postcode "0000" end end 

Null object pattern # app/models/null_address.rb class NullAddress def street "No street" end def city "No city" end def state "No state" end def postcode "0000" end end class User < ApplicationRecord def billing_address addresses.billing.first || NullAddress.new end end 

Null object pattern

Welcome, <% if current_user %> <%= current_user.name %> <% else %> Guest <% end %> ↙p> 

Null object pattern # app/controllers/application_controller.rb require "ostruct" def current_user @current_user ||= User.find session[:user_id] || OpenStruct.new(name: "Guest") end helper_method :current_user 

Null object pattern # back in the view

Welcome, <%= current_user.name %> ↙p> # app/controllers/application_controller.rb require "ostruct" def current_user @current_user ||= User.find session[:user_id] || OpenStruct.new(name: "Guest") end helper_method :current_user 

Null object pattern # app/models/guest.rb class Guest def name "Guest" end end # app/controllers/application_controller.rb def current_user @current_user ||= User.find session[:user_id] || Guest.new end helper_method :current_user 

1. Domain logic 2. Even just presentational logic can become complex 3. Difficult to maintain 4. Difficult to test 5. Not DRY ISSUES Overloading views with logic 

1. Domain logic 2. Even just presentational logic can become complex 3. Difficult to maintain 4. Difficult to test 5. Not DRY ISSUES SOLUTIONS Overloading views with logic 1. Custom view helpers 2. Presenters, decorators, view models 3. Template inheritance and nested layouts 

Another presenter example # app/presenters/customer_presenter.rb class CustomerPresenter < SimpleDelegator include ActionView::Helpers::NumberHelper def initialize(customer) super(customer) end def location AddressPresenter.new(customer.billing_address).location end def recent_order_history # do something with recent orders end def phone_number number_to_phone(phone_number, delimiter: "-") end end 

Local variables, strict locals, and magic comments # app/views/orders/show.html.erb <%= render partial: "orders/order", locals: { order: @order } %> # app/views/orders/_order.html.erb <%= tag.div id: dom_id(order) do %>

<%= order.name %>↙h1> <% end %> 

Local variables, strict locals, and magic comments # app/views/orders/show.html.erb <%= render "orders/order", order: @order %> # app/views/orders/_order.html.erb <%= tag.div id: dom_id(order) do %>

<%= order.name %>↙h1> <% end %> 

Local variables, strict locals, and magic comments # app/views/orders/show.html.erb <%= render "orders/order", order: @order %> local_assigns[:order] 

Local variables, strict locals, and magic comments # app/views/orders/show.html.erb <%# locals: (order:) -%> <%= render "orders/order" %> # => ActionView::Template::Error: missing local: :order for app/views/orders/_order.html.erb 

Fat models and God objects 1. Violates SOLID 2. Fragile code 3. Harder to test 4. Damages performance 5. Harder to understand 

Fat models and god objects # app/models/order.rb class Order < ApplicationRecord def to_xml; ... end def to_json; ... end def to_csv; ... end def to_pdf; ... end # does many more things... end 

Solution: delegate responsibility to new classes # 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 

Delegate responsibility to new classes # 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 # app/models/order.rb class Order < ApplicationRecord delegate( :to_xml, :to_json, :to_csv, :to_pdf, to: :converter ) def converter OrderConverter.new(self) end end 

Callbacks # http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html CALLBACKS = [ :after_initialize, :after_find, :after_touch, :before_validation, :after_validation, :before_save, :around_save, :after_save, :before_create, :around_create, :after_create, :before_update, :around_update, :after_update, :before_destroy, :around_destroy, :after_destroy, :after_commit, :after_rollback ] > Callbacks are hooks into the life cycle of an Active Record object that allow you to trigger logic before or after an alteration of the object state 

Callbacks class Order < ApplicationRecord belongs_to :user has_many :line_items after_create :purchase_completion_notification def purchase_completion_notification OrderMailer.purchase_notifier(self).deliver end end # in a spec allow_any_instance_of(Order). to receive(:purchase_completion_notification). and_return(true) 

Solution: use observers or service objects # app/services/complete_order.rb class CompleteOrder attr_accessor :order def initialize(order_params) @order = Order.new(order_params) end def self.call(order) new(order).call end def call if order.save OrderMailer.purchase_notifier(order).deliver end order end end 

Solution: use observers or service objects # app/services/complete_order.rb class CompleteOrder attr_accessor :order def initialize(order_params) @order = Order.new(order_params) end def self.call(order) new(order).call end def call if order.save OrderMailer.purchase_notifier(order).deliver end order end end # app/controllers/orders_controller.rb def create @order = CompleteOrder.call(params[:order]) respond_with @order end 

Separation of concerns — single responsibility # app/models/user.rb class User < ActiveRecord::Base include UserConcerns::Constants include UserConcerns::PersonalDetails include UserConcerns::Registration include UserConcerns::Pulse include UserConcerns::Enrolment include UserConcerns::Sme include Shared::DeleteSoftly Q: when breaking apart a class that has too many responsibilities, what are the pros and cons of a PORO with delegation vs a concern? Q: If you delegate, do you test that the delegation exists or only test the class that is delegated to? 

1. Move tasks to background jobs 2. Set timeouts Sluggish services — blocking on calls to external services 

Sluggish services — blocking on calls to external services # change the timeout from the default of 60 secs down to 3 uri = URI("http://example.com") request = Net::HTTP::Post.new(uri.path) http = Net::HTTP.new(uri.host, uri.port).start http.read_timeout = 3 response = http.request(request) # Faraday conn.get do |req| req.url "/search" req.options.timeout = 5 # open/read timeout in seconds req.options.open_timeout = 2 # connection open timeout in secs end # Http.rb HTTP. timeout(:per_operation, write: 2, connect: 5, read: 10). get("http://example.com") 

Failure to respond def create @order = Order.new(params[:order]) respond_to do |format| if @order.save format.xml { render xml: @order } else format.xml { render xml: @order.errors } end end end 

Solution: return explicit HTTP status codes def create @order = Order.new(params[:order]) respond_to do |format| if @order.save format.xml { render xml: @order, status: :created, } else format.xml { render xml: @order.errors, status: :unprocessable_entity, } end end end 

Controllers Views Models Requests In summary SRP! Testing & Refactoring [email protected] blackmill.co 

Thanks https://blackmill.co/coaching-training/ workshops/tdd-with-rspec Test Driven Development with RSpec Elle Meredith [email protected] blackmill.co build an effective and fast testing environment 30 July – 2 August