- 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.
- 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.
- 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.
- Continuing from #3, you can (presumably) collapse the entire things into 1 database query.
I'd probably start with something like
redirect_path = request.referrer || root_path
if current_user.students.memberships.where("group_id = ?", exception.record.id).any?
redirect_to redirect_path, alert: t('flash.pending_acceptance') and return
redirect_to redirect_path, alert: t('flash.access_denied')
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.