Download Java Code Snippet: Refactoring Rental Record Statement and more Study notes Software Engineering in PDF only on Docsity!
totalAmount += thisAmount;
// add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.tape().movie().priceCode() == Movie.NEW_RELEASE) && each.daysRented() > 1) frequentRenterPoints ++;
//show figures for this rental result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(thisAmount) + "\n";
} //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result;
}
When I've made the change the next thing is to remove the old method. The compiler should then tell me if I missed anything.
There is certainly some more I would like to do to Rental.charge() but I will leave it for the moment and return to Customer.statement().
The next thing that strikes me is that thisAmount() is now pretty redundant. It is set to the result of each.charge() and not changed afterwards. Thus I can eliminate thisAmount by replacing a temp with a query.
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement();
//determine amounts for each line totalAmount += each.charge();
// add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.tape().movie().priceCode() == Movie.NEW_RELEASE) && each.daysRented() > 1) frequentRenterPoints ++;
//show figures for this rental
result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(each.charge())
} //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result;
}
I like to get rid of temporary variables like thus as much as possible. Temps are often a problem in that they cause a lot of parameters to get passed around when they don't need to. You can easily lose track of what they are there for. They are particularly insidious in long methods. Of course there is a small performance price to pay, here the charge is now calculated twice. But it is easy to optimize that in the rental class, and you can optimize much more effectively when the code is properly refactored.
Extracting Frequent Renter Points
The next step is to do a similar thing for the frequent renter points. Again the rules vary with the tape, although there is less variation than with the charging. But it seems reasonable to put the responsibility on the rental. First we need to extract a method from the frequent renter points part of the code (highlighted below).
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement();
//determine amounts for each line totalAmount += each.charge();
// add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.tape().movie().priceCode() == Movie.NEW_RELEASE) && each.daysRented() > 1) frequentRenterPoints ++;
//show figures for this rental result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(each.charge())
} //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement();
//determine amounts for each line totalAmount += each.charge();
// add frequent renter points frequentRenterPoints += each.frequentRenterPoints();
//show figures for this rental result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(each.charge())
} //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result;
}
int frequentRenterPoints() { if ((tape().movie().priceCode() == Movie.NEW_RELEASE) && daysRented() > 1) return 2; else return 1; }
Removing Temps
As I suggested before, temporary variables can be a problem. They are only useful within their own routine, and thus they encourage long complex routines. In this case we have two temporary variables, both of which are being used to get a total from the rentals attached to the customer. Both the ascii and html versions will require these totals. I like to replace temps with queries. Queries are accessible to any method in the class, and thus encourage a cleaner design without long complex methods.
I began by replacing totalAmount with a charge() method on customer.
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement();
// add frequent renter points frequentRenterPoints += each.frequentRenterPoints();
//show figures for this rental result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(each.charge())
} //add footer lines result += "Amount owed is " + String.valueOf(charge()) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result;
}
private double charge(){ double result = 0; Enumeration rentals = _rentals.elements(); while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement(); result += each.charge(); } return result; }
After compiling and testing that refactoring, I then did the same for frequentRenterPoints.
public String statement() { Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement(); //show figures for each rental result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(each.charge()) + "\n"; } //add footer lines result += "Amount owed is " + String.valueOf(charge()) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints()) + " frequent renter points"; return result; }
private int frequentRenterPoints() { int result = 0; Enumeration rentals = _rentals.elements(); while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement(); result += each.frequentRenterPoints();
There is still some code copied from the ascii version, but that is mainly due to setting up the loop. Further refactoring could clean that up further, extracting methods for header, footer, and detail line are one route I could take. But that isnít where I want to spend my time, I would like to move onto the methods Iíve moved onto rental. Back on with the refactoring hat.
Moving the Rental Calculations to Movie
Yes itís that switch statement that is bugging me. It is a bad idea to do a switch based on an attribute of another object. If you must use a switch statement, it should be on your own data, not on someone elseís. This implies that the charge should move onto movie
Class movie Ö double charge(int daysRented) { double result = 0; switch (priceCode()) { case REGULAR: result += 2; if (daysRented > 2) result += (daysRented - 2) * 1.5; break; case NEW_RELEASE: result += daysRented * 3; break; case CHILDRENS: result += 1.5; if (daysRented > 3) result += (daysRented - 3) * 1.5; break;
} return result; }
For this to work I have to pass in the length of the rental, which of course is data due of the rental. The method effectively uses two pieces of data, the length of the rental and the type of the movie. Why do I prefer to pass the length of rental rather than the movieís type? Its because type information tends to be more volatile. I can easily imagine new types of videos appearing. If I change the movieís type I want the least ripple effect, so I prefer to calculate the charge within the movie.
I compiled the method into movie and then adjusted the charge method on rental to use the new method.
Class rentalÖ double charge() { return _tape.movie().charge(_daysRented); }
Some people would prefer to remove that chain of calls by having a charge(int) message on tape. This would lead to
Class rental double charge() { return _tape.charge(_daysRented); }
Class tape double charge() { return _movie.charge(_daysRented); }
You can make that change if you like, I donít tend to worry about message chains providing that they all lie in the same package. If they cross package boundaries, then Iím not so happy, and would add an insulating method.
Having done this with charge amounts, Iím inclined to do the same with frequent renter points. The need is less pressing, but I think it is more consistent to do them both the same way. And again if the movie classifications change it makes it easier to update the code.
Class rentalÖ int frequentRenterPoints() { return _tape.movie().frequentRenterPoints(_daysRented); }
class movieÖ int frequentRenterPoints(int daysRented){ if ((priceCode() == NEW_RELEASE) && daysRented > 1) return 2; else return 1; }
With these two changes I can hide those constants, which is generally a Good Thing. Even constant data should be private.
private static final int CHILDRENS = 2; private static final int REGULAR = 0; private static final int NEW_RELEASE = 1;
To really do this, however, I need to change a couple of other parts of the class. I need to change how we create a movie. I used to create a movie with a message like
new Movie ("Ran", Movie.REGULAR);
and the constructor
class MovieÖ private Movie(String name, int priceCode) { _name = name;
This would allow me to replace the switch statement by using polymorphism. Sadly it has one slight flaw: it doesnít work. A move can change its classification during its lifetime. An object cannot change its class during its lifetime. There is a solution however, the state pattern [Gang of Four]. With the state pattern the classes look like this.
By adding the indirection we can do the subclassing from the price code object, changing the price whenever we need to.
With a complex class you have to move data and methods around in small pieces to avoid errors, it seems slow but it is the quickest because you avoid debugging. For this case I could probably move the data and methods in one go as the whole thing is not too complicated. However Iíll do it the bit by bit way, so you can see how it goes. Just remember to do it one small bit at a time if you do this to a complicated class.
The first step is to create the new classes. Then I need to sort out how they are managed. As the diagram shows they are all singletons. It seems sensible to get hold of them via the superclass with a method like Price.regular(). I can do this by getting the superclass to manage the instances of the subclasses.
abstract class Price { static Price regular() { return _regular; }
static Price childrens() { return _childrens; } static Price newRelease() { return _newRelease; }
private static Price _childrens = new ChildrensPrice(); private static Price _newRelease = new NewReleasePrice(); private static Price _regular = new RegularPrice(); }
Now I can begin to move the data over. The first piece of data to move over is the price code. Of course Iím not actually going to use the price code within the Price object, but I will give it the illusion of doing so. That way the old methods will still work. They key is to modify those methods that access and update the price code value within Movie. My first step is to self-encapsulate the type code , ensuring that all uses of the type code go though getting and setting methods. Since most of the code came from other classes, most methods already use the getting method. However the constructors do access the price code, I can use the setting methods instead.
