• Post Reply Bookmark Topic Watch Topic
  • New Topic

Refactor duplicate methods  RSS feed

 
Mary Joe
Greenhorn
Posts: 20
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi,

I have two methods which are being called form UI to display 2 different values.

These two methods have the same code apart form one additional condition in one of them.

Is there any efficient way of refactoring them to avoid the duplication ?

First Method
---------------

private Double getTotalSum(List amounts) {
double totalToPay = 0.00;

Iterator amountsIterator = amounts.iterator();
while (amountsIterator.hasNext()) {
Amount amount = (Amount) amountsIterator.next();
if (!cancelstatuses.contains(amount.getStatus())) {
totalToPay += amount.doubleValue();
}
}
return new Double(totalToPay);
}

Second Method
=----------------


private Double getTotalSumExcludeCancelAmount(List amounts) {
double totalToPay = 0.00;

Iterator amountsIterator = amounts.iterator();
while (amountsIterator.hasNext()) {
Amount amount = (Amount) amountsIterator.next();
if (!amount.getIsToCancel()) { // Additional condition comparing to the first method.
if (!cancelstatuses.contains(amount.getStatus())) {
totalToPay += amount.doubleValue();
}
}
}
return new Double(totalToPay);
}

Thanks
M
 
Campbell Ritchie
Marshal
Posts: 56592
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Maybe, yes.

Maybe, take a second cancellable booolean parameter. Then you can add that to the if. You can use && or || depending on whether you want to bypass the check or reinforce it.
BTW: getIsToCancel is hardly a good method name. Try isCancellable or isToBeCancelled.
 
Campbell Ritchie
Marshal
Posts: 56592
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
And in the few minutes I have looked at your methods, I have seen lots besides.
  • 1: Why are you using plain List? Can you use a List<Amount> and Iterator<Amount> instead? Then you can get rid of the dangerous (Amount) cast.
  • 2: Why are you using an Iterator explicitly rather than a for‑each loop?
  • 3: Why are you mixing doubles and Doubles?
  • 4: Why are you looking in a second collection to see whether the Amount should be cancelled? What sort of collection is it? Unless it is something like a hash‑based set, you are liable to find that method runs in quadratic complexity.
  • 5: Why are you using floating‑point arithmetic for money? That is wrong. Look here for what you should use.
  •  
    Mary Joe
    Greenhorn
    Posts: 20
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thank you that was helpful
     
    Consider Paul's rocket mass heater.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!