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
def get_let(response)
let = response.css('A')
let = response.css('B') if let.empty?
let = '' if let.empty?
end
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?.
responses.detect(&:present) || ''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.
find will raise this exception: NoMethodError: undefined method 'call' for "":String. find and its alias detect require an argument that 1) is nil, or 2) responds to call. See my answer for some alternative implementations to the collection approach.
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
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
return keyword. I know return is not typically very Rubyish, but I'd make an exception in this case because using it would clarify the code.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
