From: Sylvester Keil <sylvester.keil@...>
Date: 2011-11-05T20:12:11+09:00
Subject: Re: Help me refactor my ruby code please


On Nov 5, 2011, at 11:30 AM, Pavel Varela wrote:

> Right now a have a piece of code that i use in a rails app. Take a look
> at it:
> 
> def calculate(days, size, terminal)
>  (0..days).inject {|sum, day| sum + costs(day, size, terminal)}
> end
> 
> def costs(days, size, terminal)
>  if terminal == "PLP"
>    if size == 20
>      case days
>        when 0..5; 0
>        when 6; 75
>        when 7..8; 25
>        when 9..20; 75
>        else 125
>      end
>    elsif size == 40
>      case days
>        when 0..5; 0
>        when 6; 150
>        when 7..8; 50
>        when 9..20; 150
>        else 250
>      end
>    end
>  elsif terminal == "FCT"
>    if size == 20
>      case days
>        when 0..5; 0
>        when 6; 75
>        when 7..8; 25
>        when 9..20; 115
>        when 21..30; 165
>        when 31..60; 150
>        else 135
>      end
>    elsif size == 40
>      case days
>        when 0..5; 0
>        when 6; 150
>        when 7..8; 50
>        when 9..20; 230
>        when 21..30; 330
>        when 31..60; 300
>        else 270
>      end
>    end
>  end
> end
> 
> puts calculate(14, 40, "FCT")
> 
> 
> The idea behind it's quite obvious, i think. Depending on a cost per
> day, container size and terminal i get the total cost.
> 
> That case statement looks ugly for me, though i can't find a more
> elegant way. And when i imagine 10 more terminals... The previous
> version was like (without terminal and size):
> 
> # COSTS = { 0..5 => 0, [6] => 75, [7, 8] => 25, 9..20 => 115, 21..30 =>
> 165, 31..60 => 250 }
> # COSTS.default = 0
> 
> #def calculate(days)
> #  price = 0
> #  (0..days).each do |day|
> #    COSTS.each_key do |key|
> #      price += COSTS[key] if key.include?(day)
> #    end
> #  end
> #  price
> #end
> 
> but here the calculate method is ugly...
> 
> Guys, do u have any idea what i could do with that? Thanks a lot =)

Personally, I'd prefer your older version. You can put your data in a nested hash and freeze it. Then you could use something like this calculate:

def calculate(days, size = 40, terminal = :FCT)
  (0 .. days).inject do |sum, day|
    sum + COSTS.deep_fetch(terminal, size, day, 0)
  end
end

Although a default cost of 0 is probably not in your best interest ;-)

This uses a #deep_fetch as proposed by Avdi Grimm on his blog:

http://avdi.org/devblog/2011/06/28/do-or-do-not-there-is-no-try/