Refactor Out Code Smell With JDeodrant

i got to know about jdeodorant - a tool for identifying bad smells in code and helping it to refactor. i got curious and downloaded its eclipse plugin, i then picked the first bad smell code which martin fowler explains in his book: “refactoring: improving the design of existing code”. i tried my hand at refactoring a long method by extract method refactor move. here’s the long method:

public string statement(){
  double totalamount = 0;
  int frequentrenterpoints = 0;
  string result = "rental record for "+ getname() + "\n";
  for(rental each : _rentals){
    double thisamount = 0;
    switch (each.getmovie().getpricecode()) {
      case movie.regular:
        thisamount += 2;
        if ( each.getdaysrented() > 2)
          thisamount += (each.getdaysrented() - 2 ) * 1.5;
        break;
      case movie.new_release:
        thisamount += each.getdaysrented() *3;
        break;
      case movie.children:
        thisamount += 1.5;
        if (each.getdaysrented() > 3 )
          thisamount += (each.getdaysrented() - 3)* 1.5;
        break;
    }
    frequentrenterpoints++;
    if ( (each.getmovie().getpricecode() == movie.new_release)
          && each.getdaysrented() > 1 )
      frequentrenterpoints ++;
    result += "\t" + each.getmovie().gettitle() + "\t" +
        string.valueof(thisamount) + "\n";
    totalamount += thisamount;
  }
  result += "amount owed is "+ string.valueof(totalamount)+"\n";
  result += "you earned " + string.valueof(frequentrenterpoints) +
      " frequent renter points";
  return result;
}

so i use the jdeodorant plugin and run the long method: bad smell, it correctly identifies the thisamount calculation as a long method, as shown in the image:

now i try to incorporate the suggested refactor and it leads to the code where the thisamount calculation is moved into a method getamount:

private void getamount() {
  for (rental each : _rentals) {
    double thisamount = 0;
    switch (each.getmovie().getpricecode()) {
      case movie.regulr:
        thisamount += 2;
        if (each.getdaysrented() > 2) {
          thisamount += (each.getdaysrented() - 2) * 1.5;
        }
        break;
      case movie.new_release:
        thisamount += each.getdaysrented() * 3;
        break;
      case movie.children:
        thisamount += 1.5;
        if (each.getdaysrented() > 3) {
          thisamount += (each.getdaysrented() - 3) * 1.5;
        }
        break;
    }
  }
}

and the original code is altered as:

//older code
thisamount = getamount(); //new line added after refactor
for(rental each : _rentals){
  frequentrenterpoints++;
//code here moved to getamount method
//older code
  if ( (each.getmovie().getpricecode() == movie.new_release)
      && each.getdaysrented() > 1 )
    frequentrenterpoints ++;

the identification of the parts which could be refactor was correct but the refactor done wasn’t as expected. ideally the variables which the block of code was referring to should have moved to the method param list and the code identified for refactor copied as is. and also as this refactor was around the use of a certain variable “thisamount”, the extracted method should have returned the calculated value. so i might have to explore the way the refactoring is done.

but to start with:
- it helps in identifying bad smells – i tried with the one which was obvious, not sure about the not so obvious ones.
- one can consider the refactoring advice but not always will the judgement of a tool be better than human experience. so take it with a pinch of salt.

i would like to explore this as and when i get time, but i would also have to read the ideas behind the bad smell identification and refactoring suggestions before i can move along with the tool.

just in case someone has already tried using this tool, please feel free to drop in the feedback.

and an interesting name for the project: jdeodorant- to drive away the bad smell