HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Client categorisation based on hourly payment

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
categorisationpaymenthourlyclientbased

Problem

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 = 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?

Solution

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.

Code Snippets

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"

Context

StackExchange Code Review Q#129434, answer score: 3

Revisions (0)

No revisions yet.