Client categorisation based on hourly payment

Question

I am currently working on a small internal rating system for clients to be able to prioritise my work based on what client pays best. The input data is the hourly rate they are paying and based on that they should be divided into categories in are the range from A-D. The first setup looks like this:

def category_for(client)
  hourly_rate = client.hourly_rate
  case client.country
  when :germany
    return 'A' if hourly_rate >= 35
    return 'B' if hourly_rate >= 30
    return 'C' if hourly_rate >= 25
    return 'D' if hourly_rate < 25
  when :sweden
    return 'A' if hourly_rate >= 400
    return 'B' if hourly_rate >= 350
    return 'C' if hourly_rate >= 300
    return 'D' if hourly_rate < 300
  end
end

This is ofc only part of it to show the general setup.

This code, although working fine, really really hurts my eyes, so I was wondering if someone here had a good idea on how to refactor it in a nice way?

Answer 1

I like @SergeiiK's first solution, but we can make this more declarative:

GRADES = {
  germany: {
    "A" => 35...Float::INFINITY,
    "B" => 30...35,
    "C" => 25...30,
    "D" => 0...25,
  },
  sweden: {
    "A" => 400...Float::INFINITY,
    "B" => 350...400,
    "C" => 300...350,
    "D" => 0...300,
  },
}

def category_for(client)
  GRADES[client.country].find do |_, range|
    range.cover?(client.hourly_rate)
  end.first
end

Client = Struct.new(:country, :hourly_rate) # just for demonstration

p category_for(Client.new(:germany, 26))
# => "C"

p category_for(Client.new(:sweden, 7000))
# => "A"

You'd probably want to add some error handling to category_for for invalid inputs.

Note that the inner Hashes are Hashes for aesthetics only. Using Enumerable#find on a Hash will iterate over its key-value pairs one by one just as it would an array of key-value pairs ([ [ "A", 35...Float::INFINITY ], ... ]), making lookup O(n) instead of O(1) (ish) like normal Hash lookup. This is just prettier than nested arrays. This won't be as fast as a case solution, either, but unless you're, say, "grading" hundreds of thousands of clients at once you won't notice a difference.

Answer 2

The pattern return something if condition is ok as a guard to check preconditions, but not to write the core logic of the method (IMHO), which should use expressions preferably. I think a simple case it's much more clear:

def category_for(client)
  hourly_rate = client.hourly_rate
  case client.country
  when :germany
    case
    when hourly_rate >= 35 then 'A'
    when hourly_rate >= 30 then 'B'
    when hourly_rate >= 25 then 'C'
    when hourly_rate < 25 then 'D'
    end
  when :sweden
    case
    when hourly_rate >= 400 then 'A'
    when hourly_rate >= 350 then 'B'
    when hourly_rate >= 300 then 'C'
    when hourly_rate < 300 then 'D'
    end
  end
end

Now, if this started to grow, or the intervals had to be configurable, I'd extract those values to data structures. Something like this, you get the idea:

CATEGORIES_BY_HOURLY_RATE = {
  germany: [35, 30, 25],
  sweden: [400, 350, 300],
}

Answer 3

I suggest further improvement (using ranges):

def category_for(client)
  hourly_rate = client.hourly_rate
  case client.country
  when :germany
    case hourly_rate
    when 0...25  then 'D'
    when 25...30 then 'C'
    when 30...35 then 'B'
    else              'A'
    end
  when :sweden
    case hourly_rate
    when 0...300   then 'D'
    when 300...350 then 'C'
    when 350...400 then 'B'
    else                'A'
    end
  end
end

And finally, I come with this DRY solution (to avoid nested case duplication):

def category_for(client)
  hourly_rate = client.hourly_rate
  # common_quotient acts like cross-course
  common_quotient = client.country == :germany ? 1 : 11.5
  common_hourly = hourly_rate / common_quotient

  case common_hourly
  when 0...25  then 'D'
  when 25...30 then 'C'
  when 30...35 then 'B'
  else              'A'
  end
end