Complicated views

[jarkelen (John van Arkelen)]

Hi, the biggest struggle I have with Rails is keeping my views as simple as possible. I read a lot about OO, especially Sandi Metz’s book (which is excellent by the way), and I totally understand those (simple) examples. But in real life I always end up with views like these, where you cycle through an instance variable and then add a lot of complicated stuff.

Luckily I already use service objects, otherwise it would be even more complicated, but it still feels way to bloated. Are there any good ideas to simplify this and extract some complicated stuff out of the view?

%table.table.table-striped
  %thead
    %tr
      %th= I18n.t('.users.singular')
      - (1..12).each do |month|
        %th= I18n.t("date.abbr_month_names")[month]
      %th= I18n.t('.general.total')
  %tbody
    - unless @users.blank?
      - year = params[:year].blank? ? Date.today.year : params[:year]
      - total = [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]
      - @users.each do |user|
        - user_total = 0
        %tr
          %td{ "data-title" => "#{I18n.t('.users.singular')}" }= link_to user.full_name, user_path(user)
          - (1..12).each do |month|
            - check_date = "01-#{month}-#{year}".to_date
            - if check_date <= DateTime.now
              - calculator = TimesheetCalculator.new(user.timesheets.where(month: month, year: year).first)
              - if params[:matrix_type] == "unbillable"
                - result = calculator.get_employee_unbillable_hours
              - elsif params[:matrix_type] == "holidays"
                - result = (calculator.get_employee_holidays/8).round(1)
              - elsif params[:matrix_type] == "kms"
                - result = calculator.get_employee_kms
              - else
                - result = calculator.get_employee_billable_hours

              - total[month] += result
              - user_total += result
              %td{ "data-title" => I18n.t("date.abbr_month_names")[month] }= result > 0 ? result : "-"
            - else
              %td{ "data-title" => I18n.t("date.abbr_month_names")[month] }= "-"
          %td{ "data-title" => "#{I18n.t('.general.total')}" }= user_total > 0 ? user_total : "-"

    - else
      = render "shared/placeholder", colspan: "13"
  %thead
    %tr
      %th= I18n.t('.general.total')
      - matrix_total = 0
      - (1..12).each do |month|
        - matrix_total += total[month]
        %th= total[month] unless total.blank?
      %th= matrix_total

[charlieanna]

Why haml?

[DeviousDuck (Matthew Sumner)]

@jarkelen couple of things off the top of my head:

• I would move the months header into a partial or helper to make it easier to read.
• I notice your using an unless, else. I personally find these unclear so would change it to an if-else.
• Your view is calculating user_total. Ideal your view shouldn’t do any calculations. This should already be done and then passed to the view. Keep you view stupid and it will keep itself simple.
• All the logic creating TimesheetCalculator and checking params needs to be somewhere else. First it should be moved to the controller and then refactored from there if your controller ends up bloated.

Ideally, your controller would pass your view an array with all the table values. If the logic is here in a view, it’s not usable anywhere else.

With the logic removed, your view could be:

%table.table.table-striped
  - render 'table_header_months'
  %tbody
    - # loop over array to make table rows.

@charlieanna let’s try to stay constructive. Apart from using haml, is there anything else you noticed @jarkelen could do to help clean up this view?

[geoffharcourt]

@charlieanna It’s not the reason I would use Haml (I would use it because I think it encourages good markup), but in this case with all this logic that’s not related to tag rendering, Haml does help convey that Ruby logic more clearly than if a bunch of <% %> tags were littering the markup.

[derekprior]

@jarkelen

But in real life I always end up with views like these, where you cycle through an instance variable and then add a lot of complicated stuff.

The added complicated stuff needs to happen before you get to the view. This view has a ton of business logic in it: how to handle previous/future check dates. How to handle various ‘matrix_type’ settings. The smells here are the various if statements that aren’t strictly related to display, the initialization of an object in your view, the call to where in your view, the use of local variables, etc.

I haven’t traced this code all the way through to figure out what it is its supposed to be doing, but what it sounds like you want your controller to return for you is an object that has a summation of hours worked per month for a given year, for an array of users. You don’t have to do that all in one go, looks for areas of the code where you can extract an object that the controller can prepare for you. For instance, I’d likely start by trying to eliminate all of the logic in the users.each block.

[Pierre]

but in terms of performance, iterating an array a first time in controller for preparing an array to iterate a second time in its view, it bothers me each time

[DeviousDuck (Matthew Sumner)]

In terms of performance, everything should be in one big class so we’re not instantiating so many objects =p

But in all seriousness, iterating over an array is cheap. Doing it twice, once in your controller to build an object and a second time in your view to build up the dom costs practically nothing in performance. The time it will save when you need to come back and change this logic is certainly not insignificant.

These rules, namely separating concerns, are there to make our lives easier and happier. Keep only view logic in your views and they will remain manageable and agile.