0

I'm trying to do a method for choosing an AB test option in Rails, according to a test result. I would like to find a more efficient way of doing the first nil test: If given option A is nil, return B If given option B is nil, return A

module AbTestHelper
  def ab_test_choice(ab_test_name, option_a, option_b, *args)
      #This part
      return option_b unless option_a
      return option_a unless option_b
      #This part    

      check_test_enabled = "#{ab_test_name}_enabled?"
      test_result = send(check_test_enabled, args) if respond_to? check_test_enabled
      return option_a if result
      option_b 
  end

  private

  def colour_enabled?(user = current_user)
    return unless user.is_a? User
    user.enabled_for?(:colour)
  end
end
  • What exactly is inelegant and/or inefficient about the way you're doing it? How do you define "most elegant and efficient"? – jvillian Nov 15 '17 at 18:11
  • Though this works, it seems a little inefficient to me that I have to do the same type of check for both options. I would like an more efficient way of eliminating nil options. I have 2 here, but if I had more it would be a problem testing one by one. –  Nov 15 '17 at 18:15
  • It seems fine to me. If I were you, I would move on. BTW, you don't seem to ever set `result` (you do seem to set `test_result`), so `return option_a if result` will never return `option_a` and you will always return `option_b`. Unless I'm missing something... – jvillian Nov 15 '17 at 18:23
  • 3
    Working code is kinda off-topic here. This question might be a better fit in [Code Review](http://codereview.stackexchange.com). – Mark Thomas Nov 15 '17 at 18:45
  • To be more precise in your test I would use `return option_b if option_a.nil? and `return option_a if option_b.nil?` to check for `nil` explicitly. There are non-nil values that are falsey. – Tom Aranda Nov 15 '17 at 18:48
  • What sort of values do `option_a` and `option_b` get? If this is just boolean values, should be easy. In Ruby the only possibilities for logically false are literal `false` and `nil`. That last chunk of code in the method only runs if the two possible values are logically false, so it's very odd how they interact. – tadman Nov 15 '17 at 20:46
  • I can guarantee that at least one of the options will be truthy, otherwise calling the test doesn't make sense in my application context. Apologies for asking the question in the wrong place, I didn't know about Code Review –  Nov 16 '17 at 15:44

1 Answers1

1

It seems ok to me but an alternative way is to use the ^ operator like this:

...
return option_a or option_b if !!option_a ^ !!option_b
...

The ^ operator is an exclusive or, so it would be true only if one of the options is true or not nil.

The problem with this is it's less idiomatic and harder to understand than the original version.

Bastian E.
  • 408
  • 2
  • 11
  • This doesn't work when `option_a` and `option_b` are both `nil` (or more generally, both falsy). btw, I don't see why `!!` is needed. – Cary Swoveland Nov 15 '17 at 20:55
  • You are right about both options being falsy. And the `!!` is needed only on the first parameter because the `^` can lead to errors without it: https://stackoverflow.com/a/1058502/2877645 – Bastian E. Nov 16 '17 at 22:15
  • That's a good point about `!!`. An alternative would be `option_a.nil? ^ option_b.nil?`, which, unlike `!!`, returns the correct result when `option_a = 'cat'` and `option_b = false` (namely, `false`). – Cary Swoveland Nov 17 '17 at 02:44