Ruby on Rails application controller

Question

My application controller keeps growing with small helper methods that I use in both my controller and views, is it okay? or what do you recommend doing?

class ApplicationController < ActionController::Base
 protect_from_forgery with: :exception
 helper_method :resource_name, :resource, :devise_mapping, :resource_class, :is_admin?, :is_representative?, :is_shipper?,
 :is_agent?, :is_operation_completed?, :is_fcl_exw?, :is_fcl_exw_info_stage_completed?, :is_fcl_exw_info_requested?, :is_fcl_exw_info_confirmed?,
 :is_pricing_representative?, :is_fcl_exw_quotation_confirmed?
 # Roles detection helpers
 def is_admin?
 current_role == 'admin' ? true : false
 end
 def is_representative?
 current_role == 'representative' ? true : false
 end
 def is_shipper?
 current_role == 'shipper' ? true : false
 end
 def is_agent?
 current_role == 'agent' ? true : false
 end
 def is_pricing_representative?
 current_role == 'pricing_representative' ? true : false
 end
 # Devise helpers
 def resource_name
 :user
 end
 def resource
 @resource ||= User.new
 end
 def resource_class
 User
 end
 def devise_mapping
 @devise_mapping ||= Devise.mappings[:user]
 end
 # Operation helpers
 def is_operation_completed?(operation_id)
 GeneralCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
 end
 # FCL-EXW helpers
 def is_fcl_exw_info_stage_completed?(operation_id)
 FclExwCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
 end
 def is_fcl_exw?(operation_id)
 Operation.find(operation_id).modality == 'FCL - EXW' ? true : false
 end
 def is_fcl_exw_info_requested?(operation_id)
 Operation.find(operation_id).fcl_exw_info_requested
 end
 def is_fcl_exw_info_confirmed?(operation_id)
 Operation.find(operation_id).fcl_exw_info_confirmed
 end
 def is_fcl_exw_quotation_confirmed?(operation_id)
 Operation.find(operation_id).fcl_exw_quotation_confirmed
 end
 private
 def current_role
 current_role = current_user.nil? ? 'Not logged in' : Role.find(current_user.role_id).name 
 end
 def require_new_operation_permission
 check_permission('representative', 'agent')
 end
 def check_permission(*roles)
 unless roles.include? current_role
 flash[:alert] = "Access denied"
 redirect_back(fallback_location: authenticated_root_path)
 end
 end
end

Answer 1

There is no need to use the prefix is with every method if your method return type is boolean then simply use ? at the end of the method name I replace all the role methods name from is_role? with role? ,i.e is_admin? to admin? etc. instead of nil? Should use present?

class ApplicationController < ActionController::Base
 protect_from_forgery with: :exception
 helper_method :resource_name, :resource, :devise_mapping, :resource_class, :is_operation_completed?, :is_fcl_exw?, :is_fcl_exw_info_stage_completed?, :is_fcl_exw_info_requested?, :is_fcl_exw_info_confirmed?,:is_fcl_exw_quotation_confirmed?
 ROLES = %w[admin representative shipper agent pricing_representative]
 ROLES.each do |role|
 define_method("#{role}?") { current_role == role }
 helper_method "#{role}?".to_sym
 end
 # Devise helpers
 def resource_name
 :user
 end
 def resource
 @resource ||= User.new
 end
 def resource_class
 User
 end
 def devise_mapping
 @devise_mapping ||= Devise.mappings[:user]
 end
 # Operation helpers
 def is_operation_completed?(operation_id)
 GeneralCargoInfo.find_by(operation_id: operation_id).present?
 end
 # FCL-EXW helpers
 def is_fcl_exw_info_stage_completed?(operation_id)
 FclExwCargoInfo.find_by(operation_id: operation_id).present?
 end
 def is_fcl_exw?(operation_id)
 Operation.find(operation_id).modality == 'FCL - EXW'
 end
 def is_fcl_exw_info_requested?(operation_id)
 Operation.find(operation_id).fcl_exw_info_requested
 end
 def is_fcl_exw_info_confirmed?(operation_id)
 Operation.find(operation_id).fcl_exw_info_confirmed
 end
 def is_fcl_exw_quotation_confirmed?(operation_id)
 Operation.find(operation_id).fcl_exw_quotation_confirmed
 end
 private
 def current_role
 current_role = current_user.nil? ? 'Not logged in' : Role.find(current_user.role_id).name 
 end
 def require_new_operation_permission
 check_permission('representative', 'agent')
 end
 def check_permission(*roles)
 unless roles.include? current_role
 flash[:alert] = "Access denied"
 redirect_back(fallback_location: authenticated_root_path)
 end
 end
end

Note: remove is with other methods as well also if you not consume this helper methods with any of the controllers then move them to view helper

Answer 2

You can move helper methods to ApplicationHelper and simply include it in Application controller, so your code will look like:

ApplicationController:

class ApplicationController < ActionController::Base
 include ApplicationHelper
 protect_from_forgery with: :exception
 private
 def current_role
 current_role = current_user.nil? ? 'Not logged in' : Role.find(current_user.role_id).name 
 end
 def require_new_operation_permission
 check_permission('representative', 'agent')
 end
 def check_permission(*roles)
 unless roles.include? current_role
 flash[:alert] = "Access denied"
 redirect_back(fallback_location: authenticated_root_path)
 end
 end
end

ApplicationHelper:

module ApplicationHelper
 # Roles detection helpers
 def is_admin?
 current_role == 'admin' ? true : false
 end
 def is_representative?
 current_role == 'representative' ? true : false
 end
 def is_shipper?
 current_role == 'shipper' ? true : false
 end
 def is_agent?
 current_role == 'agent' ? true : false
 end
 def is_pricing_representative?
 current_role == 'pricing_representative' ? true : false
 end
 # Devise helpers
 def resource_name
 :user
 end
 def resource
 @resource ||= User.new
 end
 def resource_class
 User
 end
 def devise_mapping
 @devise_mapping ||= Devise.mappings[:user]
 end
 # Operation helpers
 def is_operation_completed?(operation_id)
 GeneralCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
 end
 # FCL-EXW helpers
 def is_fcl_exw_info_stage_completed?(operation_id)
 FclExwCargoInfo.find_by(operation_id: operation_id).nil? ? false : true
 end
 def is_fcl_exw?(operation_id)
 Operation.find(operation_id).modality == 'FCL - EXW' ? true : false
 end
 def is_fcl_exw_info_requested?(operation_id)
 Operation.find(operation_id).fcl_exw_info_requested
 end
 def is_fcl_exw_info_confirmed?(operation_id)
 Operation.find(operation_id).fcl_exw_info_confirmed
 end
 def is_fcl_exw_quotation_confirmed?(operation_id)
 Operation.find(operation_id).fcl_exw_quotation_confirmed
 end
end

• Dan Oberlam: You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process.
• Sebastian Delgado: The problem with this is that I need to access those helpers from my controllers, adding them in a helper module would make them accessible only for my views.