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.

Transcript

1. Exploring Rails Anti-Patterns Elle Meredith elle@blackmill.co
2. • Controllers • Views • Models • Requests What’s the plan doc?
3. Our little shop app
4. Common approaches to recurring problems that ultimately prove to be ineffective What are anti-patterns?
5. 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?
6. Fat controllers
7. 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
8. 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
9. 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
10. 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?
11. 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?
12. 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
13. 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
14. 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
15. 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>
16. 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
17. 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"]]
18. 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"]]
19. 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"]]
20. 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?
21. 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
22. 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 %>
23. 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)
24. 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
25. 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?
26. 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
27. 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 %>
28. Dealing with nils undefined method `street' for nil:NilClass @order.try(:customer) .try(:billing_address) .try(:street)
29. 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 %>
30. 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
31. 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
32. Null object pattern <p> Welcome, <% if current_user %> <%= current_user.name %> <% else %> Guest <% end %> ↙p>
33. 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
34. 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
35. 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
36. 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
37. 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
38. 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
39. 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 %>
40. 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 %>
41. Local variables, strict locals, and magic comments # app/views/orders/show.html.erb <%= render "orders/order", order: @order %> local_assigns[:order]
42. 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
43. Fat models and God objects 1. Violates SOLID 2. Fragile code 3. Harder to test 4. Damages performance 5. Harder to understand
44. 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
45. 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
46. 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
47. 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
48. 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)
49. 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
50. 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
51. 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?
52. 1. Move tasks to background jobs 2. Set timeouts Sluggish services — blocking on calls to external services
53. 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")
54. 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
55. 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
56. Controllers Views Models Requests In summary SRP! Testing & Refactoring elle@blackmill.co blackmill.co
57. Thanks https://blackmill.co/coaching-training/ workshops/tdd-with-rspec Test Driven Development with RSpec Elle Meredith elle@blackmill.co blackmill.co build an effective and fast testing environment 30 July – 2 August