Rubocop claims for too many 'if' statements

Question

I am measuring my code for a Rails app using Rubocop. This is my code:

def ratings_treatment
    if @rating.advertise_id
      advertise = Advertise.find(advertise_id)
      rating_average(advertise)
    elsif @rating.product_id
      product = Product.find(product_id)
      rating_average(product)
    elsif @rating.combo_id
      combo = Combo.find(combo_id)
      rating_average(combo)
    else
      establishment = Establishment.find(@rating.establishment_id)
      rating_average(establishment)
    end   
end

It does not pass a test regarding if. It is claimed that there are too many if statements.

I thought of splitting the code into some checker methods, but was wondering if there is some better way to get good metrics and still write good code.

What does RuboCop actually say? I'm pretty sure it's not "Too many if statements". — Stefan Pochmann, Dec 28 '17 at 02:52

Sebastián Palma · Accepted Answer · 2017-12-28T02:30:08.003

I think you could use a switch statement to assign the argument for rating_average:

def ratings_treatment
  average = case
            when @rating.advertise_id then Advertise.find(advertise_id)
            when @rating.product_id   then Product.find(product_id)
            when @rating.combo_id     then Combo.find(combo_id)
            else Establishment.find(@rating.establishment_id)
            end
  rating_average average
end

Although Rubocop will complain with

Style/EmptyCaseCondition: Do not use empty case condition, instead use an if expression

You could also simplify your if expression:

def ratings_treatment
  average = if @rating.advertise_id
              Advertise.find advertise_id
            elsif @rating.product_id
              Product.find product_id
            elsif @rating.combo_id
              Combo.find combo_id
            else
              Establishment.find @rating.establishment_id
            end
  rating_average average
end

score 3 · Answer 2 · answered Dec 28 '17 at 03:25

If your @rating has associations set up then you could say:

def ratings_treatment
  obj = @rating.advertise || @rating.product || @rating.combo || @rating.establishment
  rating_average(obj)
end

or even:

def ratings_treatment
  rating_average(@rating.advertise || @rating.product || @rating.combo || @rating.establishment)
end

or perhaps something fancy (and overkill for only four branches):

def ratings_treatment
  rating_average(
    %w[advertise product combo establishment].lazy.map { |m| @rating.send(m) }.find(&:present?)
  )
end

or maybe even:

def rated_object
  to_result = ->(m) { @rating.send(m) }
  %w[advertise product combo establishment]
    .lazy
    .map(&to_result)
    .find(&:present?)
end

def ratings_treatment
  rating_average(rated_object)
end

I'd go so far as to suggest that something like the above rated_object method really should be a method on @rating. You should be able to ask a rating for the thing that was rated without having to manually spin through all the possibilities.

You could even patch that lazy.map... stuff into Enumerable if you wanted something in Enumerable that behaved like e.m1 || e.m2 || e.m3 || ... (been there, done that) all the way down to short circuiting.

I have no idea what Rubocop would make of any of these shenanigans though.

@sawa But doesn't that `find` call give you the first `m` where `@rating.send(m)` is truthy rather than the first truthy *result* of `@rating.send(m)`? i.e. you'd get one of the strings `'advertise'`, `'product'`, ... rather than the result of calling the named method. — mu is too short, Dec 28 '17 at 03:34
@sawa And the `#presence` call isn't necessary since the association methods shouldn't be returning empty strings or `false` or anything like that. — mu is too short, Dec 28 '17 at 03:38

score 0 · Answer 3 · answered Dec 28 '17 at 10:46

This is a classic case of a style violation that has a deeper underlying cause, i.e. a modelling problem. In your case, it looks like what you're actually trying to achieve is a polymorphic association. If you change your Rating to:

# app/models/rating.rb
class Rating < ApplicationRecord
  belongs_to :rateable, polymorphic: true
end

you can have the Rating relate to any number of things, by simply adding this to the other end of the association:

# app/models/product.rb
class Product < ApplicationRecord
  has_many :pictures, as: :rateable
end

Your original method, if it is even needed anymore, becomes:

def ratings_treatment
  rating_average(@rating.rateable) 
end

Rubocop claims for too many 'if' statements

3 Answers3