How to refactor already simple ruby code

Question

I want to refactor this ruby code

class UtilService


  def summer_start
    ...
  end

  def summer_end
    ...
  end 

  def calc(date, charge, quantity, winter_rate, winter_service_charge, summer_rate)
    if date < summer_start || date > summer_end
      chargeamt = quantity * winter_rate + winter_service_charge
    else
      chargeamt = quantity * summer_rate
    end
    chargeamt
  end
end

Since charge isn't being used I know I can get rid of that but other than getting rid of that one parameter I can't think of what else I can actually do.

I was also thinking of breaking up 'calc' into different methods

class UtilService
  def summer_start
  end

  def summer_end
  end

  def calc_summer_rate(date, quantity, summer_rate)
    if date > summer_start || date < summer_end
      chargeamt = quantity * summer_rate
    end
    chargeamt
  end

  def calc_winter_rate(date, quantity, winter_rate, winter_service_charge)
    if date < summer_start || date > summer_end
      chargeamt = quantity * winter_rate + winter_service_charge
    end
    chargeamt
  end

end

But it doesn't seem like my refactoring makes much sense.

Why the rush to select an answer?

Cary Swoveland
– Cary Swoveland

2017-07-21 07:32:56 +00:00
Commented Jul 21, 2017 at 7:32 — Cary Swoveland
– Cary Swoveland, Commented Jul 21, 2017 at 7:32

Cary Swoveland · Accepted Answer · 2017-07-21 13:15:40Z

3

I would be inclined to write something like the following.

rates = { winter: { per_unit: 123, service_charge: 456 },
          summer: { per_unit: 135, service_charge: 0   } }

def calc(date, quantity, rates)
  rate = summer?(date) ? rates[:summer] : rates[:winter]
  quantity * rate[:per_unit] + rate[:service_charge]
end

def summer?(date)
  date.between?(summer_start, summer_end)
end

edited Jul 21, 2017 at 13:15

answered Jul 21, 2017 at 7:32

Cary Swoveland

111k6 gold badges69 silver badges105 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Gerry Over a year ago

I like this approach, it feels more organized (and flexible with rates); just a couple of typos: rates(:summer) to rates[:summer] and per_unit to rate[:per_unit].

Cary Swoveland Over a year ago

@Gerry, thanks for both the compliment and corrections. My answers always seem to be riddled with errors when I can't or don't test. ¯\_(ツ)_/¯

Gerry · Accepted Answer · 2017-07-21 07:08:00Z

2

I don't see big refactoring needed, maybe just moving the is summer? logic to another method and removing unnecesary chargeamt variable; something like this:

class UtilService
  def summer_start
  end

  def summer_end
  end

  def calc(date, quantity, winter_rate, winter_service_charge, summer_rate)
    if summer?(date)
      quantity * winter_rate + winter_service_charge
    else
      quantity * summer_rate
    end
  end

  private
  def summer?(date)
    date > summer_start || date < summer_end
  end
end

edited Jul 21, 2017 at 7:08

answered Jul 21, 2017 at 7:03

Gerry

10.5k3 gold badges33 silver badges40 bronze badges

8 Comments

user2914144 Over a year ago

Hmm, wouldn't there be an issue with date being a local var?

Gerry Over a year ago

@user2914144 Yes, thanks for the catch. I've updated the answer.

user2914144 Over a year ago

Sweet thanks. I'm testing this just to see if it's all good and I'm getting an error that says "comparison of Fixnum with nil failed" any idea what the issue is?

user2914144 Over a year ago

But i think since you got rid of chargeamt, it's returning a boolean

Gerry Over a year ago

@user2914144 No, ruby returns the last expression evaluated when no explicit return is provided; so it will either return the result of the expression in if or the one in else, both numbers. How are you testing your code?

|

Collectives™ on Stack Overflow

How to refactor already simple ruby code

2 Answers 2

2 Comments

8 Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

8 Comments

Related