Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

According to my lecture notes, there are benefits to refactoring the code from original code to refactor code.

Reasons for refactoring:

Replace Temp with Query

  • You are using many temporary variables to hold the result of an expression
  • This can result in long methods, since the (temp) values are available only in local scope
  • Queries, in the form of method invocation, means that the value is available from any method in the class.

Look for a temporary variable that is assigned to once

  • Declare the temp as final
  • Compile
  • Extract the right-hand side of the assignment into a method
  • Compile and test

Original code

double basePrice = _quantity * _itemPrice;

if (basePrice > 1000)
return basePrice * 0.95;
else
return basePrice * 0.98;

Refactored code

if (basePrice() > 1000)
return basePrice() * 0.95;
else
return basePrice() * 0.98;
...
double basePrice() {
return _quantity * _itemPrice;
}

What are the benefits from this refactor? Please enlightened me as I can't see what the benefits to refactoring the code are in this case.

share|improve this question

closed as off-topic by Marc-Andre, rolfl, Fge, amon, Jamal Mar 4 at 16:07

If this question can be reworded to fit the rules in the help center, please edit the question.

    
Honestly, neither can I. Are you sure this is your lecture's notes? Refactoring basePrice into a method and calling that twice only makes things worse IMO, in case it's a multithreaded environment and a value has changed in between the calls. –  Simon André Forsberg Mar 4 at 14:59
6  
This question appears to be off-topic because the answer is not yes to all the on-topic questions. Especially "Did I write that code?" and "Is it actual code from a project rather than pseudo-code or example code?" I think Programmers is a better place for this question. –  Simon André Forsberg Mar 4 at 15:03
    
Actually we're having a small discussion in Code Review Chat concerning that.. you are welcome to join –  Vogel612 Mar 4 at 15:06
    
@all I have added the reasons as to why my lecture notes suggested the refactor –  Computernerd Mar 4 at 15:11
2  
Because it's a binary floating point type, and thus can't represent values like 0.1 or 0.98 exactly. This propagates to wrong rounding. In most languages double can also give different results depending on the machine or global rounding modes. –  CodesInChaos Mar 4 at 15:18

1 Answer 1

up vote 7 down vote accepted

You have misunderstood one thing that your lecturer said, extracting a calculation to a method does not mean that you should call it multiple times within the same method!

You are probably doing double basePrice = _quantity * _itemPrice; on more than one place in your code, the idea is that each of those places should be double basePrice = calcBasePrice(); which makes your code above:

double basePrice = calcBasePrice();
if (basePrice > 1000)
    return basePrice * 0.95;
else
    return basePrice * 0.98;

double calcBasePrice() {
    return _quantity * _itemPrice;
}

Other suggestions:

  • To perform calculations on money, you want exact precision. double does not have exact percision. You should use BigDecimal instead.

  • basePrice() is not a good method name, I changed that in my code above to calcBasePrice()

  • Ident your code properly, as I've done above.

Also, this code:

if (basePrice > 1000)
    return basePrice * 0.95;
else
    return basePrice * 0.98;

Can use the ternary operator:

return basePrice > 1000 ? basePrice * 0.95 : basePrice * 0.98;

Or to make it clearer, you can use a factor variable.

double factor = (basePrice > 1000 ? 0.95 : 0.98);
return basePrice * factor;
share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.