0
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
0
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Dannnno May 23 '18 at 13:47
  • \$\begingroup\$ 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. \$\endgroup\$ – Sebastian Delgado May 24 '18 at 14:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.