CODEWITHSUNDEEP
ruby
Ben Edwards
Ben Edwards

Reputation: 455

ruby dealing with nil in complex if

I have a if which looks at 5 variables and some of them can be nil. I have the following code working but it feels really messy. It uses the assumption that the string being tested is not 100 *, which it can not be, but regardless of this is there a better way of doing this. 

m['search2']  = '*' * 100 if m['search2'].nil?
m['search3']  = '*' * 100 if m['search3'].nil?
m['exclude1'] = '*' * 100 if m['exclude1'].nil?
m['exclude2'] = '*' * 100 if m['exclude2'].nil?

if ( @title.upcase.index( m['search1'] ) || @title.upcase.index( m['search2']) || @title.upcase.index( m['search3']) ) &&
   !(@title.upcase.index( m['exclude1']) || @title.upcase.index( m['exclude1']) ) then
  exclude = true
end

Just to clarify, in view of the answers dealing with better ways to set defaults, while this is great I was hoping I could do something clever within the if as it was the actually having to set the defaults what felt messy.

Upvotes: 0

Views: 69

Answers (4)

Holger Just
Holger Just

Reputation: 55888

It seems you are trying to work around an error you got on String#index when passing it nil as an argument instead of the String it expected.

In that case, there is a much simpler (and more idiomatic solution) if you think about what you actually want to achieve. In your case, it appears you want to set exclude to true if the @title includes any of the optionally given search terms and excludes any of the optionally given exclude terms.

This can be achieved with the following method. Note that it could be simplified if instead of a Hash with "magic" key names, you would give your terms as two arrays, one for the search terms and one for the exclude terms. Then, your method would also be extendable for more terms without having to be adapted.

def exclude?(m)
  # upcase the @title only once for performance
  upcased_title = @title.upcase

  # Depending on the m argument, you might be able to further
  # improve this to get rid of the repetition 
  exclude_terms = [
    m['exclude1'],
    m['exclude2']
  ].compact
  # ^^^^^^^ Compacting on an array gets rid on any nil values

  # Same here.
  search_terms = [
    m['search1'],
    m['search2'],
    m['search3']
  ].compact
  # ^^^^^^^ Same for the search terms

  # We use include? instead of index to better convey our meaning
  return false if exclude_terms.any? { |term| upcased_title.include?(term) }
  return true if search_terms.any? { |term| upcased_title.include?(term) }

  # TODO: what should happen in neither an exclude term nor
  # a search term was found?
end

Note that the conditions seem to be backwards in your question. There you are setting exclude to false if a search term matches but no exclude term did. You probably mean the exact opposite of that.

Upvotes: 1

tadman
tadman

Reputation: 211720

Unless you're dealing with a situation where, for whatever reason, one of those values might be literal false then your life is very convenient. Just define some defaults:

DEFAULT_TERM = '*' * 100
DEFAULTS = {
  'search2' => DEFAULT_TERM,
  'search3' => DEFAULT_TERM,
  'exclude1' => DEFAULT_TERM,
  'exclude2' => DEFAULT_TERM
}

Then later you can merge that in:

m = DEFAULTS.merge(m)

Anything that's not defined in m will automatically be left as the default. You may need to clean up m first:

m = DEFAULTS.merge(m.compact)

Where that removes any nil values.

Another thing to note is to omit spurious syntax like then. It's not necessary, and largely unused.

If you reorganized your structure into something like this you'd have a much easier time of implementing a more flexible search:

{
  'search' => [ 'term1', 'term2', 'term3' ],
  'exclude' => [ 'exclude1', 'exclude2', 'exclude3' ]
}

Since then you can use simple iteration.

Upvotes: 1

you786
you786

Reputation: 3550

Anytime you have repeated code, you should always think of using loops or methods. Here's an example of using loops so that you can avoid rewriting 

search_keys = ['search2', 'search3']
exclude_keys = ['exclude1', 'exclude2']

search_keys.each do |key|
  m[key] = '*' * 100 if m[key].nil?
end

exclude_keys.each do |key|
  m[key] = '*' * 100 if m[key].nil?
end

Upvotes: 1

mahemoff
mahemoff

Reputation: 46509

Start with a simplification:

m['search2']  = '*' * 100 if m['search2'].nil?

is better expressed as:

m['search2'] ||= '*' * 100

Then make the whole thing a loop (also make the string a constant for clarity):

STARS = '*' * 100

%w(search2 search3 exclude3 exclude2).each { |key|
  m[key] ||= STARS
}

Though if you're only doing this as a default, there's a better way as tadman notes.

Upvotes: 1

Related Questions