CODEWITHSUNDEEP
ruby
yeenow123
yeenow123

Reputation: 836

Ruby Way of Nested If Statement

Right now I have I feel like a very ugly way of setting a variable to a value depending on if it returns an empty string or not. Below is the method in question (it uses Nokogiri, but that doesn't really matter for this question).

def get_let(response)
    if response.css('A').empty?
        if response.css('B').empty?
            let = ''
        end
        let = response.css('B')
    else
        let = response.css('A')
    end

    return let
end

Upvotes: 2

Views: 1274

Answers (6)

Michael Stalker
Michael Stalker

Reputation: 1367

This isn't quite as readable as @sethcall's answer, but it should be be fairly readable if you know some Ruby idioms:

def get_let(response)
  responses = [response.css('A'), response.css('B')]
  responses.detect { |response| !response.empty? } || ''
end

detect returns the first result for which the block doesn't return false. This has the advantage of avoiding conditionals, if that's something you're going for. If you want to do without the || in the above answer, you could do this:

def get_let(response)
  responses = [response.css('A'), response.css('B')]
  responses.detect(-> { '' }) { |response| !response.empty? }
end

I don't find that second solution to be nearly as intuitive as the first solution, though. It would be terrific if you could just specify an empty string as an argument. However, the argument for detect and its alias, find, must be nil or something that responds to the call method, like a lambda or proc. There's really no reason to pass in nil, since that's the default value.

If you knew for sure that the response.css method would not return an array with nil or false values in it, you could try this solution:

def get_let(response)
  responses = [response.css('A'), response.css('B')]
  responses.detect(&:any?) || ''
end

See the Ruby docs to read more about how detect works. Here are the docs on any?.

Upvotes: 3

Maurício Linhares
Maurício Linhares

Reputation: 40313

Remove return and let:

def get_let(response)
    if response.css('A').empty?
        response.css('B').empty? ? '' : response.css('B')
    else
        response.css('A')
    end
end

Or with the use of collections:

def get_let(response)
    ['A', 'B'].map { |l| response.css(l) }.find { |items| !items.empty? } || ''
end

Avoids computing the CSS selector twice.

Upvotes: 2

AndyV
AndyV

Reputation: 3741

def get_let(response)
  case
    when !response.css('A').empty?
      response.css('A')
    when response.css('A').empty? && !response.css('B').empty?
      response.css('B')
    else
     ''
  end
end

Upvotes: 0

CDub
CDub

Reputation: 13344

You don't need let - just have it return the results of the if/else block... I prefer to keep it like this as it's easier to read than ternaries...

def get_let(response)
  if response.css('A').empty?
    if response.css('B').empty?
      ''
    else
      response.css('B')
    end
  else
    response.css('A')
  end
end

Upvotes: 0

sethcall
sethcall

Reputation: 2897

def get_let(response)
    let = response.css('A')
    let = response.css('B') if let.empty?
    let = '' if let.empty?
end

Upvotes: 4

Chris Cherry
Chris Cherry

Reputation: 28554

I would do something like this, very explicit what is going on, no branching logic to keep track of while reasoning about it.

def get_let(response)
    return '' if response.css('A').empty? && response.css('B').empty?
    return response.css('B') if response.css('A').empty?
    return response.css('A') if response.css('B').empty?
end

Upvotes: 0

Related Questions