Refactoring custom validator using lots of if's

Question

I have a custom validator that checks the min and max score of different sporting leagues -- e.g., MLB, NBA, NFL, NCAAB, etc.

Here's short version of what it looks like:

class ScoreValidator < ActiveModel::Validator

  def validate(event)

    if event.league_is_mlb?
      if event.total_score < 1
        event.errors.add :base, 'min total score for an MLB event is 1'
      elsif event.total_score > 49
        event.errors.add :base, 'max total score for an MLB event is 49'
      end
    elsif event.league_is_nba?
      if event.total_score < 119
        event.errors.add :base, 'min total score for an NBA event is 119'
      elsif event.total_score > 370
        event.errors.add :base, 'max total score for an NBA event is 370'
      end
    end

  end

end

Seems like this code could be much cleaner by making smarter use of ruby data structures and methods. Any creative suggestions you could make would be educational and much appreciated.

Answer 1

1. You can refactor each condition using extract method

   def validate_mlb_score
   def validate_nba_score
   

2. Also you can methods to determine min/max score

   def max_score_for(event)
     event.league_is_mlb? ? 49 : 370 # judging by your elseif without else I've suggested about only two leagues
   end
   

I think my code would look like:

def validate(event)
  if event.total_score < event.min_score
    event.errors.add :base, "min total score for an #{event.name} event is #{event.min_score}"
  elsif event.total_score > event.max_score
    event.errors.add :base, "max total score for an #{event.name} event is #{event.max_score}"
  end
end

# I also suggest what you have this kind of model
class Event < ActiveRecord::Base

  def min_score
    self.league_is_mlb? ? 1 : 119
  end

  def max_score
    self.league_is_mlb? ? 49 : 370
  end
end