Hi, I setup a presenter using a Plain Old Ruby Object to remove logic out of my Rails views. I did so I could easily specify in the RSpec tests that the reasons were being reported for unpayable tickets. Is the only way to make this neater/cleaner/avoid the nesting and still preserve the tests is by trying to move some of the table creation “h.content_tag” to their own private helper methods?
class TicketReportPresenter
def ticket_report
if @model.tickets.any?
h.content_tag(:h4, "Users have tickets").
concat(
h.content_tag(:table, class: "table table-hover") do
h.content_tag(:thead) do
h.content_tag(:th, "User Email").
concat h.content_tag(:th, "Reason(s)")
end
h.content_tag(:tbody) do
@model.unpayable_tickets.each do |payment|
h.concat(
h.content_tag(:tr) do
h.content_tag(:td, payment.email).
concat(
h.content_tag(:td) do
h.content_tag(:ul) do
payment.reasons.each do |reason|
h.content_tag(:li, reason)
end
end
end
)
end
)
end
end
end
)
end
end
end
Moving logic out of the Rails views using presenters/helpers is a good practice.
The good news is that there is plenty of techniques you can use to refactor your presenter to make it (1) clearer to read with less nesting and (2) more easily testable.
Here are some suggestions
Use “Extract Method” to pull out logical bits of your code into separate methods (as you mentioned). It’s really easy to do this when working with HTML because the methods you want to pull out usually correspond to the different parts of the markup structure. For example header, table, table_body, etc. In addition to making the code more clear, you can now also individually test each method. Ruby Science: Extract Method | Online Video Tutorial by thoughtbot
Use safe_join for concatenating elements. This method takes an array and combines them all into a single buffer for use in the view. For example, you can concatenate your header and table like this safe_join [<header, table]. safe_join (ActionView::Helpers::OutputSafetyHelper) - APIdock
Use delegate to reduce some code. You can remove the repeated calls to helper methods on the h object (e.g. h.content_tag) by using delegate to make it an implicit call within your presenter. For example, delegate :content_tag, :concat, to: :h. This will allow you to call content_tag inside your presenter and it will automatically resolve to h.content_tag. delegate (Module) - APIdock
Use a guard clause. Instead of if @model.tickets.any? (with the happy path nested underneath), use a guard clause like return if @model.tickets.empty? Prefer Guard Clauses over nested conditionals
Use consistent naming. A minor tip, but in your iteration over @model.unpayable_tickets you name the iteration variable payment even though the collection is called unpayable_tickets. Why not name the variable ticket?
With these tips in mind, here’s a Gist of a possible refactoring of your method:
Lastly, you may find this blog post I wrote about Rails helpers relevant. It utilizes many of the same techniques: