5
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ I really like your approach, thx for sharing! \$\endgroup\$ – Severin May 30 '16 at 7:26
2
\$\begingroup\$

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],
}
\$\endgroup\$
1
\$\begingroup\$

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
\$\endgroup\$
6
  • 2
    \$\begingroup\$ I like the ranges. I can't say that I like the currency conversion formula, though. \$\endgroup\$ – 200_success May 27 '16 at 8:23
  • \$\begingroup\$ Me neither, but it's really not good idea to nest multiple case statements. If you'll add another currency quotient, it will just affect one if/case statement. \$\endgroup\$ – Sergii K May 27 '16 at 8:28
  • \$\begingroup\$ The problem is, the thresholds aren't proportional, and your conversion isn't exact, so you could get the wrong answer for non-integer rates. \$\endgroup\$ – 200_success May 27 '16 at 8:34
  • \$\begingroup\$ Thx for your suggestion. I quite like the general approach, but I agree with @200_success on the conversion formula. Especially if I wanna add more currencies this creates an additional layer of complexity. \$\endgroup\$ – Severin May 27 '16 at 11:26
  • 1
    \$\begingroup\$ If you pay for work you've done - that is really something goes wrong, don't you think? \$\endgroup\$ – Sergii K May 28 '16 at 4:58

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.