How to write a nested search the Ruby way?

Question

I am trying to write fast and concise code. I'd appreciate your thoughts on which is the best way to write the following code and why:

Option #1

def get_title
  title = check_in_place_one
  if title.empty?
    title = check_in_place_two
    if title.empty?
      title = check_in_place_three
    end
  end
  return title
end

Option #2

def get_title
  title = check_in_place_one
  title = check_in_place_two unless !title.empty?
  title = check_in_place_three unless !title.empty?
  return title
end

I think Option #1 is better since if the title is found by check_in_place_one, we test title.empty? once and then skip the rest of the code in the method and return. But, it looks too long. Option #2 appears better, but processes title.empty? one extra time, and unnecessary time before returning. Also, am I missing a third option?

Answer 1

The following approach could be used for any number of sequential tests. Moreover, it is completely general. The return condition could be changed, arguments could easily be assigned to the test methods, etc.

tests = %w[check_in_place_one check_in_place_two check_in_place_three]

def do_tests(tests)
  title = nil # Define title outside block
  tests.each do |t|
    title = send(t)
    break unless title.empty?
  end
  title
end

Let's try it:

def check_in_place_one
  puts "check 1"
  []
end

def check_in_place_two
  puts "check 2"
  ''
end

def check_in_place_three
  puts "check 3"
  [3]
end

do_tests(tests) #=> [3]
check 1
check 2
check 3
  #=> [3]

Now change one of the tests:

def check_in_place_two
  puts "check 2"
  'cat'
end

do_tests(tests) #=> 'cat'
check 1
check 2
  #=> "cat"         

If there were more tests, it might be convenient to put them in a module which would be included into a class. Mixed-in methods behave the same as those that you define for the class. For example, they have access to instance variables. I will demonstrate that with the definition of the first test method. We probably want to make the test methods private. We could do it like this:

module TestMethods
  private

  def check_in_place_one
    puts "@pet => #{@pet}"
    puts "check 1"
    []
  end

  def check_in_place_two
    puts "check 2"
    ''
  end

  def check_in_place_three
    puts "check 3"
    [3]
  end
end

class MyClass  
  @@tests = TestMethods.private_instance_methods(false)
  puts "@@tests = #{@@tests}"

  def initialize
    @pet = 'dog'
  end

  def do_tests
    title = nil # Define title outside block
    @@tests.each do |t|
      title = send(t)
      break unless title.empty?
    end
    title
  end   

  include TestMethods       
end

The following is displayed when the code is parsed:

@@tests = [:check_in_place_one, :check_in_place_two, :check_in_place_three]

Now we perform the tests:

MyClass.new.do_tests #=> [3]
@pet => dog
check 1
check 2
check 3 

Confirm the test methods are private:

MyClass.new.check_in_place_one
  #=> private method 'check_in_place_one' called for...'

The advantage of using a module is that you can add, delete, rearrange and rename the test methods without making any changes to the class.

Answer 2

I don't think there's any reason to get too clever:

def get_title
  check_in_place_one || check_in_place_two || check_in_place_three
end

Edit: if the check_in_place_X methods are indeed returning an empty string on failure it would be better (and more idiomatic) to have them instead return nil. Not only does it allow for truthy comparisons like the above code, return "" results in the construction of a new and unnecessary String object.

Answer 3

Well, here's a few other alternatives.

Option 1: Return first non-empty check.

def get_title
  return check_in_place_one   unless check_in_place_one.empty?
  return check_in_place_two   unless check_in_place_two.empty?
  return check_in_place_three
end

Option 2: Helper method with short-circuit evaluation.

def get_title
  check_place("one") || check_place("two") || check_place("three")
end

private

def check_place(place)
  result = send("check_in_place_#{place}")
  result.empty? ? nil : result
end

Option 3: Check all places then find the first that's non-empty.

def get_title
  [
    check_in_place_one,
    check_in_place_two,
    check_in_place_three,
  ].find{|x| !x.empty? }
end

Answer 4

From performance, there is no difference between the two versions of your code (besides very minor difference that may come from parsing, which should be ignorable). The control structures are the same.

From readability, if you can get away with nesting, doing so is better. Your second option is better.

It is usually better to get rid of any case that does not need further processing. That is done by return.

def get_title
  title = check_in_place_one
  return title unless title.empty?
  title = check_in_place_two
  return title unless title.empty?
  title = check_in_place_three
  return title
end

The last title = and return in the code above are redundant, but I put them there for consistency, which improves readability.

You can further compact the code using tap like this:

def get_title
  check_in_place_one.tap{|s| return s unless s.empty?}
  check_in_place_two.tap{|s| return s unless s.empty?}
  check_in_place_three
end

tap is a pretty much fast method, and unlike instance_eval, its performance penalty is usually ignorable.

Answer 5

Option 2 looks good although you did a 360 degree turn with the unless !title.empty?. You can shorten that to if title.empty? since unless is equivalent to an if ! so doing an unless ! takes you back to just if.

If you're only ever going to have 3 places to look in then option 2 is the best. It's short, concise, and easy to read (easier once you fix the aforementioned whoopsie). If you might add on to the places you look for a title in you can get a bit more dynamic:

def get_title(num_places = 4)
  title, cur_place = nil, 0
  title = check_in_place(cur_place += 1) while title.nil? && cur_place < num_places
end

def check_in_place(place_num)
  # some code to check in the place # given by place_num
end

The tricky line is that one with the while in it. What's happening is that the while will check the expression title.nil? && cur_place < num_places and return true because the title is still nil and 0 is less than 4.

Then we'll call the check_in_place method and pass in a value of 1 because the cur_place += 1 expression will increment its value to 1 and then return it, giving it to the method (assuming we want to start checking in place 1, of course).

This will repeat until either check_in_place returns a non nil value, or we run out of places to check.

Now the get_title method is shorter and will automatically support checking in num_places places given that your check_in_place method can also look in more places.

One more thing, you might like to give https://codereview.stackexchange.com/ a look, this question seems like it'd be a good fit for it.