Avatar

very uglish method, please help

1
2
3
4
5
6
7
8
9
10
11
12
13
14
def calculate_price_of_credits(credits)
  case credits
  when 1..5000
    (credits * 0.01)
  when 5001..10000
    (5000 * 0.01) + ((credits - 5000) * 0.008)
  when 10001..25000
    (5000 * 0.01) + (5000 * 0.008) + ((credits - 10000) * 0.007)
  when 25001..50000
    (5000 * 0.01) + (5000 * 0.008) + (15000 * 0.007) + ((credits - 25000) * 0.006)
  when 50001..(1.0/0) # hack na zwrocenie obiektu Infinity
    (5000 * 0.01) + (5000 * 0.008) + (15000 * 0.007) + (25000 * 0.006) + ((credits - 50000) * 0.005)
  end
end

Refactorings

No refactoring yet !

5170ca260dbd2cdfd5a887a4dba7636f

Jeremy Weiskotten

March 7, 2008, March 07, 2008 21:21, permalink

No rating. Login to rate!

Here's a refactoring with a unit test.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
def calculate_price_of_credits(credits)
  rates = [ 0.005, 0.006, 0.007, 0.008, 0.01 ]
  tiers = [ 50000, 25000, 10000,  5000,    0 ]
  price = 0
  tiers.each_with_index do |tier, index|
    if credits > tier
      price += (rates[index] * (credits - tier))
      credits = tier
    end
  end
  price
end


class CreditTest < Test::Unit::TestCase
  def test_calc
    assert_equal 0.01, calculate_price_of_credits(1)
    assert_equal 50, calculate_price_of_credits(5000)
    
    assert_equal 50.008, calculate_price_of_credits(5001)
    assert_equal 90.0, calculate_price_of_credits(10000)
    
    assert_equal 90.007, calculate_price_of_credits(10001)
    assert_equal 195.0, calculate_price_of_credits(25000)
    
    assert_equal 195.006, calculate_price_of_credits(25001)
    assert_equal 345.0, calculate_price_of_credits(50000)

    assert_equal 345.005, calculate_price_of_credits(50001)
    assert_equal 595.0, calculate_price_of_credits(100000)
  end
end
Avatar

Ryan Bates

March 7, 2008, March 07, 2008 21:53, permalink

No rating. Login to rate!

Instead of hard coding the numbers you could move this to a database table. One column could be the tier (0, 5000, 10000, etc.) and another be the rate (0.01, 0.008, etc.). This way it will be easier to change if you ever need to. Also, if you're using Rails, this will give you a nice ActiveRecord model to move the logic into.

5170ca260dbd2cdfd5a887a4dba7636f

Jeremy Weiskotten

March 8, 2008, March 08, 2008 01:30, permalink

No rating. Login to rate!

A slightly more condensed version... This also allows you to pass in the tier/rate structure, and could pretty easily be adapted to use an ActiveRecord model instead as Ryan suggested. (Love the Railscasts!)

1
2
3
4
5
6
7
8
9
10
11
12
def calculate_price_of_credits(credits, rates=nil)
  rates ||= [ [50000, 0.005], [25000, 0.006], [10000, 0.007], [5000, 0.008], [0, 0.01] ]

  price = 0
  rates.each do |tier, rate|
    if credits > tier
      price += (rate * (credits - tier))
      credits = tier
    end
  end
  price
end

Your refactoring





Format Copy from initial code

or Cancel