Is there any DRYer way to write this method ? or is it better to keep it this way for better reader understanding ...

Each role has a list of authorised activities for a specific resource ( in this case Sheet) , a user may have multiple roles ( depending upon his company, his working group in the company, his function in the working group). This method is a sheet_policy, if among all roles , the current user can "index" sheets, the method returns true otherwise it returns false

   def index?
    # check if sheet#index is listed in current_user roles activities
    sheet_activities = []
    user.roles.each {|role| sheet_activities << role.activities["sheet"]}
    return false if sheet_activities.first.nil? # no authorized activities on Sheet

activities is a JSONB attribute of the Sheet record ...

create_table :roles do |t|
  t.string      :company, null: false, default: Settings.company.default.name
  t.string      :group
  t.string      :function
  t.jsonb       :activities, null: false, default: '{}'      
  t.timestamps  null: false

Seeding example :

   @role_accounting_employee = Role.create( group: "accounting", function: "employee")  # default company : "internal"
   @role_accounting_employee.activities.deep_merge!({sheet: %w(show index) })
  • \$\begingroup\$ Why do you check sheet_activities.first.nil?? Is there anything special about the user's first role? \$\endgroup\$ Commented Sep 19, 2015 at 1:43
  • \$\begingroup\$ the :activities field is a JSONB structure which is defaulted to an empty hash. It can be filled dynamically later with authorised activities on different resources ( :sheet is one of them , but not necessarily present... ) so my first test in this case is to test if any allowed activities for the :sheet resource exist... :using .each I got a sheet.activities being [nil] Using .map should avoid it... \$\endgroup\$
    – erwin
    Commented Sep 19, 2015 at 16:24

1 Answer 1


[Improved with flat_map thanks to @tokland.]

How about:

def index?
  user.roles.flat_map { |role| role.activities['sheet'] }.include?('index')

In Ruby, it is almost never necessary to set a temporary variable to empty and iterate over an object, given the power of methods like map, reduce, find, all?, etc. I don't see the need to use uniq! since it doesn't change whether index will be present or not. It's unclear why you're checking for nil on the first sheet_activities (since you can't guarantee what will be first without sorting), but if you just want to return false when sheet_activities are nil, the above will already do so.

Also note that Rubocop likes single quotes when you're not interpolating.

  • \$\begingroup\$ about first sheet_activities.nil? see my comment above... I also agree that .uniq! is not necessary , as I'll only check for the presence of a method (:index, ..:show..) so I don't care for duplicated methods... I juts thought it was "pretty' :-)) Thanks a lot ... \$\endgroup\$
    – erwin
    Commented Sep 19, 2015 at 16:25
  • \$\begingroup\$ map + flatten(1) = flat_map \$\endgroup\$
    – tokland
    Commented Sep 20, 2015 at 21:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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