1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$
  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
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I like your suggestion on using extract method to make clearer. \$\endgroup\$ – keruilin Jan 30 '12 at 20:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.