4
\$\begingroup\$
  def user_not_authorized(exception)
    message = t('flash.access_denied')
    if exception.policy.class.to_s.underscore == 'group_policy' && current_user.students.size != 0
        current_user.students.each do |student|
          student.memberships.each do |membership|
            if membership.group_id == exception.record.id
              message = t('flash.pending_acceptance')
              break # and I would like to break from the outer loop too
            end
          end
        end
    end
    flash[:alert] = message
    redirect_to request.referrer || root_path
  end

This looks ugly to me. Is there a way to somehow compress those two each loops, where one should break both of them conditionally?

\$\endgroup\$
4
\$\begingroup\$
  1. There's no reason (as far as I can tell) to do string comparison to check a class. #is_a? would presumably work fine too. And it'd be stricter, which is desirable.
  2. There's no reason to check students.size. If there are no students, the loop just won't do anything, so why bother checking first? Conversely, if there are any students you're actually doing 2 database queries: One to get the count/size of students, and another to fetch the actual records.
  3. Presuming non-ancient Rails, you can simply say current_user.students.memberships to get all the memberships in 1 query (admittedly with a couple of JOINs), rather than loop through students individually, and then getting their memberships.
  4. Continuing from #3, you can (presumably) collapse the entire things into 1 database query.

I'd probably start with something like

def user_not_authorized(exception)
  redirect_path = request.referrer || root_path

  if exception.policy.is_a?(GroupPolicy)
    if current_user.students.memberships.where("group_id = ?", exception.record.id).any?
      redirect_to redirect_path, alert: t('flash.pending_acceptance') and return
    end
  end

  redirect_to redirect_path, alert: t('flash.access_denied')
end

Note that it doesn't actually load any records at all; it just checks if they're there or not.

In practice, I'd consider breaking it into separate methods where possible. It's not that great to have a naked query referencing a specific column name in the middle of your method.

Edit: Updated the code to return if the first redirect occurs, since you'd otherwise do two redirects in one action, which is not allowed. Thanks to tokland for catching that. Now the and return fix is about as quick and dirty as is gets, but check the comments for better ideas.

\$\endgroup\$
  • \$\begingroup\$ Seems nice. However, there is an error undefined method memberships, so the current_user.students.memberships solution doesn't work. \$\endgroup\$ – pmichna Mar 20 '15 at 8:07
  • 1
    \$\begingroup\$ @pmichna In that case, add a has_many :student_memberships, through: :students, source: :memberships to the user model, and you can simply say current_user.student_memberships \$\endgroup\$ – Flambino Mar 20 '15 at 9:05
  • 1
    \$\begingroup\$ Two minor notes: 1) I'd move all this DB logic to models. Of course, I understand you are just trying to show what's the idea. 2) As I'm a fanatic of structured conditional branches, IMO the last redirect should go in an else. \$\endgroup\$ – tokland Mar 20 '15 at 11:47
  • \$\begingroup\$ @tokland Definitely agree with regard to moving stuff to models. But which one(s) should have responsibility for tying all this together? I didn't dare guess, hence the vague note about "breaking it into separate methods". As for #2, it's a good point. I'll leave it as-is, because the alternatives are to duplicate the last redirect line, or combine the conditions into one horribly long line. \$\endgroup\$ – Flambino Mar 20 '15 at 13:09
  • 1
    \$\begingroup\$ @tokland Oops, you're correct of course! You can do a redirect_to ... and return, which is what I was thinking of, but didn't actually write... I'm so used to conditional redirects either being simple if record.save-stuff, or that they're in filters. I'll fix that, though it should of course be an else branch, as you said. \$\endgroup\$ – Flambino Mar 20 '15 at 13:42
1
\$\begingroup\$

I moved the test into its own method to separate the logic. This still shows a Rubocop error of Assignment Branch Condition size for pending? is too high. [18.25/15] which you could fix by moving the nested iteration into its own method, but that struck me as more confusing than helpful. Note that creating the pending? method allows us to use both return and next to break out of the loop at different levels. Also note the use of a guard clause.

def user_not_authorized(exception)
  flash[:alert] = if pending?(exception)
                    t('flash.pending_acceptance')
                  else
                    t('flash.access_denied')
                  end
  redirect_to request.referrer || root_path
end

def pending?(exception)
  if exception.policy.class.to_s.underscore == 'group_policy' &&
     current_user.students.size != 0
    current_user.students.each do |student|
      student.memberships.each do |membership|
        next unless membership.group_id == exception.record.id
        return true
      end
    end
  end
  false
end
\$\endgroup\$

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.