CODEWITHSUNDEEP
ruby
mr_muscle
mr_muscle

Reputation: 2900

Ruby || and && operators return wrong results without guard

I want to create Pundit policy class but operators || and && won't work as expected (Ruby 2.4.5). I've got below policy:

  def show?
    phone_present? && login.physician? \
      || login.administrator? \
      || login.delegate? \
      || (record.id == login.user.id) \
      || (!login.user.registry_card&.admin_actioned? \
           && login.user.has_patient?(record) \
           && login.user.approved_caregiver?)
  end

  def phone_present?
    record.user_demographic.phone.present?
  end

My idea is to check if phone number exists for current user - if not it should return false and then the rest logic. It turned out when the second part of && :

login.physician? \
      || login.administrator? \
      || login.delegate? \)

return true whole expression becomes true even when phone_present? is `false. The behavior is incomprehensible because:

2.4.5 :092 > a = false && true
 => false

Even weirder is that if I put guard in there everything will work as it should:

  def show?
    return false unless phone_present?

    login.physician? \
      || login.administrator? \
      || login.delegate? \
      || (record.id == login.user.id) \
      || (!login.user.registry_card&.admin_actioned? \
           && login.user.has_patient?(record) \
           && login.user.approved_caregiver?)
  end

Why does the last example work and the first one does not? I don't wanna have guard inside show? method because it doesn't seem consistent to me.

Upvotes: 0

Views: 69

Answers (2)

spickermann
spickermann

Reputation: 106792

The problem is that order in which Ruby evaluated the code is not how you expect it to do.

A simplified example would be

a && b || c

You seem to think that if a == true then false should be returned. That means you think it evaluates the code like this

a && (b || c)

But this is not the case. Because of Ruby's operator precedence the code is evaluated like this:

(a && b) || c

That means to fix the issue you need to tell Ruby that you want it to evaluate the code slightly different than the default by adding some parentheses.

def show?
  phone_present? && (
    login.physician? \
    || login.administrator? \
    || login.delegate? \
    || (record.id == login.user.id) \
    || (
      !login.user.registry_card&.admin_actioned? \
      && login.user.has_patient?(record) \
      && login.user.approved_caregiver?
    )
  )
end

Btw most of the conditions depend on login and record. I think your code would improve when you mode those conditions into a method on login. For example like this:

def has_access_to?(record)
  physician? || administrator? || delegate? \\
    || (record.id == user.id) \
    || (
      !user.registry_card&.admin_actioned? \
      && user.has_patient?(record) \
      && user.approved_caregiver?
    )
end

Which you could then use like this in the Pundit policy:

def show?
  phone_present? && login.has_access_to?(record)
end

Upvotes: 3

Holger Just
Holger Just

Reputation: 55718

Ruby's operators have a certain precedence order and thus bind depending on this order (similar to how in math formulas - and ruby - multiplication bind stringer than addition.

Important for your case her eis that the && operator binds stronger than the || operator. Your rule thus uses following implicit precedences (expressed by adding parentheses):

(phone_present? && login.physician?) \
  || (login.administrator?) \
  || (login.delegate?) \
  || (record.id == login.user.id) \
  || (!login.user.registry_card&.admin_actioned? \
       && login.user.has_patient?(record) \
       && login.user.approved_caregiver?)

The first option is thus only true if there is a phonenumber and the user is a physician. The phonenumber requirement is ignored for any of the other options.

You can resolve this either with a guard or by appropriate parentheses.

Stylewise, I personally prefer to use smaller conditions and several guards. This makes the individual rules clearer and simpler to reason about:

def show?
  return false unless phone_present?

  return true if login.physician?
  return true if login.administrator?
  return true if login.delegate?
  return true if record.id == login.user.id

  return true if !login.user.registry_card&.admin_actioned? && login.user.has_patient?(record) && login.user.approved_caregiver?)
  
  false
end

If appropriate, you may also some of the rules (such as the last one) to an additional appropriately named method. As explained above, this makes reasoning about those rules much easier and also simplifies testing.

Upvotes: 3

Related Questions