CODEWITHSUNDEEP
ruby-on-rails
user2936314
user2936314

Reputation: 1792

Working with conditionals in rails helpers

I have a simple helper in my rails app that sets a css class based on the state of an object, like:

<li class='<%= car_color(car.state) %>'><%= car.model %></li>

in the helper it's basically:

  module CarHelper
    def car_color(state)
      if state == 'in service'
        'car car-in-service'
      elsif state == 'in garage'
        'car car-in-garage'
      else
        'car'
      end
    end
  end

And it works fine for my usecase. However, how there's a new requirement that a User with a role of customer should not see the color coding that this helper creates.

My first thought was to do a check in the controller to see if the user should be able to see the color coding:

class CarsController < ApplicationController
  before_action :is_customer?
  # bunch of restful stuff

  private
  def is_customer?
    @customer if current_user.roles.include? 'customer'
  end
end

And then in my helper, I can just add a line:

def car_color(color)
  return 'car' if @customer
end

This meets the requirements, but it smells to me. Now my helper has a dependency on @customer which is passed simply because it is an instance variable. An alternative would be to explicitly pass in a user.role to car_color or to wrap all of the calls to car_color in conditionals based on the user.role, which seems even worse.

Is there a way to help prepare this code for even more conditionals based on different user roles? My thought is to do something like:

  module CarHelper
    def car_color(args)
      set_color_for_role(args[:user_role]) if args[:user_role]
      set_color_for_state(args[:car_state]) if args[:car_state]
    end

    private
    def set_color_for_role(user_role)
      # stuff
    end
    def set_color_for_state(car_state)
      # stuff
    end
  end

I don't use rails helpers very often since I mostly work in angular and am wondering if I'm missing a cleaner OOP approach.

Upvotes: 1

Views: 77

Answers (2)

Shadwell
Shadwell

Reputation: 34774

I don't see any issue with checking the current user's roles in the helper method.

You could move the checking behaviour to the user model though which would make things cleaner (you may of course want to generalise this for multiple roles):

class User < ActiveRecord::Base
  def is_customer?
    self.roles.include?('customer')
  end
end

Then in your helper you can just check if current_user.is_customer?

def car_color(state)
  if current_user.is_customer?
    'car'
  else
    if state == 'in service'
      'car car-in-service'
    elsif state == 'in garage'
      'car car-in-garage'
    else
      'car'
    end
  end

I find it useful sometimes to build up an array of the classes too which is often cleaner (I've thrown in a case too):

def car_color(state)
  car_classes = ['car']
  unless current_user.is_customer?
    car_classes << case state
      when 'in service' then 'car-in-service'
      when 'in garage' then 'car-in-garage'
    end
  end
  car_classes.join(" ")
end

Upvotes: 2

lx00st
lx00st

Reputation: 1596

Use the draper https://github.com/drapergem/draper gem and move this logic to decorator

Upvotes: 2

Related Questions