CODEWITHSUNDEEP
rubycoding-style
nickcoxdotme
nickcoxdotme

Reputation: 6697

Ruby: Combine two similar methods into one?

I have two extremely similar methods in my Ruby object for a Rails app. I know they can be combined, I just don't know how. (Extra points if you can find a more beautiful way to handle possible nils besides return unless, not using #try.)

 def is_portal_admin?(resource)
    return unless resource.user && resource.user.clinic_memberships.any?
    memberships = resource.user.clinic_memberships.collect { |membership| membership.portal_admin? }
    memberships.include?(true)
  end

  def is_staff_admin?(resource)
    return unless resource.user && resource.user.clinic_memberships.any?
    memberships = resource.user.clinic_memberships.collect { |membership| membership.staff_admin? }
    memberships.include?(true)
  end

Upvotes: 3

Views: 382

Answers (4)

tokland
tokland

Reputation: 67850

def is_portal_admin?(resource)
  is_admin_of_type?(resource, :portal)
end

def is_staff_admin?(resource)
  is_admin_of_type?(resource, :staff)
end

def is_admin_of_type?(resource, type)
  if (user = resource.user)
    user.clinic_memberships.any? { |ms| ms.send("#{type}_admin?") }
  end
end
  • It's redundant to check if there are memberships.
  • You can add || false after the conditinal so you make sure your method? returns a boolean.
  • You can make is_admin_of_type? private.

Upvotes: 0

Michael Lang
Michael Lang

Reputation: 1038

def is_admin?(resource, kind)
  if resource.user && resource.user.clinic_memberships.any?
    !!resource.user.clinic_memberships.detect { |membership| membership.send("#{kind}_admin?") }
  end
end

If you don't enter the if branch, nil is returned, so doing your conditional like above will yield the same result as an explicit return unless...

Add a second parameter and pass :staff or :portal (or "staff" or "portal"). Using "send" will eval at run time into "staff_admin?" or "portal_admin?"

Using detect instead of collect + include? will return an object if at least one found and the !! double negates to turn it into a true/false result.

I'd personally simply do it this way since that resource.user.clinic_memberships.any? is moot in grand scheme of things:

def is_admin?(resource, kind)
  !!resource.user.clinic_memberships.detect { |membership| membership.send("#{kind}_admin?") } if resource.user
end

If you're actually trying to guard against clinic_memberships being nil, then you do need the 2nd half of the conditional, but drop the ".any?", otherwise you'll get an error testing any? against nil.

Upvotes: 0

Holger Just
Holger Just

Reputation: 55758

Instead of your collect and include? mechanism, you can simply use any?. If clinic_memberships always return an array (which it does if it is e.g. a has_many association), you don't even need to check that.

def has_membership?(resource, &block)
  return unless resource.user
  resource.user.clinic_memberships.any?(&block)
end

This can then be called like

has_membership?(resource, &:portal_admin?)

which is equivalent to

has_memberhsip?(resource){|m| m.portal_admin?}

Upvotes: 2

Linuxios
Linuxios

Reputation: 35803

How about:

def is_admin_of_type?(type, resource)
  return unless resource.user && resource.user.clinic_memberships.any? && type
  memberships = resource.user.clinic_memberships.collect { |membership| membership.send("#{type}_admin?") }
  memberships.include?(true)
end

If someone gives a nonexistent type, it'l throw NoMethodError. Also, it's forward compatible if you add more admin types.

Upvotes: 3

Related Questions