Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Exploring anti-patterns in Rails

Exploring anti-patterns in Rails

Is your Rails app sluggish? Does its performance suffer? Even with the best intentions, Rails applications can become littered with anti-patterns that negatively affect developer velocity, code clarity, and maintenance. Following a few simple patterns and strategies, you can transform your Rails application code to become more robust and functional. One that will be easier to extend and maintain. This talk will explore anti-patterns in practice and suggest fixes to ensure your Rails project is a success.

Elle Meredith

April 11, 2024
Tweet

More Decks by Elle Meredith

Other Decks in Programming

Transcript

  1. 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?
  2. 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
  3. 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
  4. 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
  5. 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?
  6. 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?
  7. 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
  8. 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
  9. 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
  10. Spaghetti SQL — long queries # app/views/orders/show.html.erb <h3>Line items↙h3> <%

    line_items = LineItem .where(order: @order, name: "Computer") .or(LineItem.where(order: @order, name: "Phone")) %> <ul> <%- line_items.each do |line_item| %> <li><%= line_item.name %>↙li> <%- end %> ↙ul>
  11. 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
  12. 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"]]
  13. 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"]]
  14. 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"]]
  15. 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?
  16. 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
  17. 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 %>
  18. 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)
  19. 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
  20. 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?
  21. 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
  22. 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 %>
  23. Dealing with nils # app/views/orders/show.html.erb <%- if order.customer.billing_address.present? %> <p>

    Customer billing address: ... ↙p> <%- else %> <p>No address↙p> <%- end %>
  24. 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
  25. 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
  26. Null object pattern <p> Welcome, <% if current_user %> <%=

    current_user.name %> <% else %> Guest <% end %> ↙p>
  27. 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
  28. Null object pattern # back in the view <p> 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
  29. 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
  30. 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
  31. 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
  32. 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
  33. 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 %> <h1><%= order.name %>↙h1> <% end %>
  34. 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 %> <h1><%= order.name %>↙h1> <% end %>
  35. Local variables, strict locals, and magic comments # app/views/orders/show.html.erb <%=

    render "orders/order", order: @order %> local_assigns[:order]
  36. 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
  37. Fat models and God objects 1. Violates SOLID 2. Fragile

    code 3. Harder to test 4. Damages performance 5. Harder to understand
  38. 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
  39. 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
  40. 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
  41. 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
  42. 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)
  43. 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
  44. 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
  45. 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?
  46. 1. Move tasks to background jobs 2. Set timeouts Sluggish

    services — blocking on calls to external services
  47. 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")
  48. 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
  49. 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