Moving the amount calculation

As I look at amountOf, I can see that it uses information from the rental, but does not use information from the customer. This method is thus on the wrong object, it should be moved to the rental. To move a method you first copy the code over to rental, adjust it to fit in its new home and compile.

Class Rental

double charge() {

double result = 0;

switch (tape().movie().priceCode()) {

case Movie.REGULAR:

result += 2;

if (daysRented() > 2)

result += (daysRented() - 2) * 1.5;

break;

case Movie.NEW_RELEASE:

result += daysRented() * 3;

break;

case Movie.CHILDRENS:

result += 1.5;

if (daysRented() > 3)

result += (daysRented() - 3) * 1.5;

break;

}

return result;

}

In this case fitting into its new home means removing the parameter.

The next step is to find every reference to the old method, and adjusting the reference to use the new method. In this case this step is easy as we just created the method and it is in only one place. In general, however, you need to do a find across all the classes that might be using that method.

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.

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).

Again we look at the use of locally scoped variables. Again it uses each, which can be passed in as a parameter. The other temp used is frequentRenterPoints. In this case frequentRenterPoints does have a value beforehand. The body of the extracted method doesn't read the value, however, so we don't need to pass it in as a parameter as long as we use an appending assignment.

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.

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();

}

return result;

}

It is worth stopping and thinking a bit about this refactoring. Most refactoring reduce the amount of code, but this one increases it. That's because Java requires a lot of statements to set up a summing loop. Even a simple summing loop with one line of code per element needs six lines of support around it. Itís an idiom that is obvious to any programmer but it is noise that hides what the intent of the loop is. As Java develops and builds up its ability to handle block closures in the style of Smalltalk, I expect that overhead to decrease, probably to the single line that such an expression would take in Smalltalk.

The other concern with this refactoring lies in performance. The old code executed the while loop once, the new code executes it three times. If the while loop takes time, this might significantly impair performance. Many programmers would not do this refactoring simply for this reason. But note the words "if" and "might". While some loops do cause performance issues, most do not. So while refactoring donít worry about this. When you optimize you will have to worry about it, but you will then be in a much better position to do something about it, and you will have more options to optimize effectively. (For a good discussion on why it is better to write clearly first and then optimize, see [McConnell, Code Complete].

These queries are now available to any code written in the customer class. Indeed they can easily be added to the interface of the class should other parts of the system need this information. Without queries like these, other methods need to deal with knowing about the rentals and building the loops. In a complex system that will lead to much more code to write and maintain.

You can see the difference immediately with the htmlStatement(). I am now at the point where I take off my refactoring hat and put on my adding function hat. I can write htmlStatement()like this (and add an appropriate test).

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;

_priceCode = priceCode;

}

To keep this type code hidden I need some creation methods.

public static Movie newNewRelease(String name){

return new Movie (name, NEW_RELEASE);

}

public static Movie newRegular(String name){

return new Movie (name, REGULAR);

}

public static Movie newChildrens(String name) {

return new Movie (name, CHILDRENS);

}

Now I create a new movie with

Movie.newRegular("Monty Python and the Holy Grail");

Movies can change their classification. I change a movieís classification with

aMovie.setPriceCode(Movie.REGULAR);

I will need to add a bunch of methods to handle the changes of classification.

public void beRegular() {

_priceCode = REGULAR;

}

public void beNewRelease() {

_priceCode = NEW_RELEASE;

}

public void beChildrens() {

_priceCode = CHILDRENS;

}

Itís a bit of effort to set up these methods, but they are a much more explicit interface then the type codes. Just looking at the name of the method tells you what kind of movie you are getting. This makes the code more understandable. The trade off is that each time I add a price code I have to add a creation and update method. If I had lots of price codes this would hurt (so I wouldnít do it). If I have a few, however, then itís quite reasonable.

At lastÖ inheritance

So we have several types of movie, which have different ways of answering the same question. This sounds like a job for subclasses. We could have three subclasses of movie, each of which can have its own version of charge.

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.

public static Movie newNewRelease(String name){

Movie result = new Movie (name);

result.beNewRelease();

return result;

}

public static Movie newRegular(String name){

Movie result = new Movie (name);

result.beRegular();

return result;

}

public static Movie newChildrens(String name) {

Movie result = new Movie (name);

result.beChildrens();

return result;

}

private Movie(String name) {

_name = name;

}

After compiling and testing I now change getting and setting methods to use the new class.

public void beRegular() {

_price = Price.regular();

}

public void beNewRelease() {

_price = Price.newRelease();

}

public void beChildrens() {

_price = Price.childrens();

}

public int priceCode() {

return _price.priceCode();

}

And provide the priceCode methods on Price and its subclasses.

Class PriceÖ

abstract int priceCode();

Class RegularPriceÖ

int priceCode(){

return Movie.REGULAR;

}

To do this I need to make the constants non-private again. This is fine, I donít mind them having a little fame before they bite the dust.

I can now compile and test and the more complex methods donít realize the world has changed.

After moving the data I can now start moving methods. My prime target is the charge() method. It is simple to move.

Class MovieÖ

double charge(int daysRented) {

return _price.charge(daysRented);

}

Class PriceÖ

double charge(int daysRented) {

double result = 0;

switch (priceCode()) {

case Movie.REGULAR:

result += 2;

if (daysRented > 2)

result += (daysRented - 2) * 1.5;

break;

case Movie.NEW_RELEASE:

result += daysRented * 3;

break;

case Movie.CHILDRENS:

result += 1.5;

if (daysRented > 3)

result += (daysRented - 3) * 1.5;

break;

}

return result;

}

Once it is moved I can start replacing the case statement with inheritance. I do this by taking one leg of the case statement at a time, and creating an overriding method. I start with RegularPrice.

Class RegularPriceÖ

double charge(int daysRented){

double result = 2;

if (daysRented > 2)

result += (daysRented - 2) * 1.5;

return result;

}

This will override the parent case statement, which I just leave as it is. I compile and test for this case, then take the next leg, compile and testÖ. (To make sure Iím executing the subclass code, I like to throw in a deliberate bug and run it to ensure the tests blow up. Not that Iím paranoid or anything.)

Class ChildrensPrice

double charge(int daysRented){

double result = 1.5;

if (daysRented > 3)

result += (daysRented - 3) * 1.5;

return result;

}

Class NewReleasePriceÖ

double charge(int daysRented){

return daysRented * 3;

}

When Iíve done that with all the legs, I declare the Price.charge() method abstract.

Class PriceÖ

abstract double charge(int daysRented);

I can now do the same procedure with frequentRenterPoints(). First I move the method over to Price.

Class MovieÖ

int frequentRenterPoints(int daysRented){

return _price.frequentRenterPoints(daysRented);

}

Class PriceÖ

int frequentRenterPoints(int daysRented){

if ((priceCode() == Movie.NEW_RELEASE) && daysRented > 1) return 2;

else return 1;

}

In this case, however I wonít make the superclass method abstract. Instead I will create an overriding method for new releases, and leave a defined method (as the default) on the superclass.

Class NewReleasePrice

int frequentRenterPoints(int daysRented){

return (daysRented > 1) ?

2:

1;

}

Class PriceÖ

int frequentRenterPoints(int daysRented){

return 1;

}

Now I have removed all the methods that needed a price code. So I can get rid of the price code methods and data on both Movie and Price.

Putting in the state pattern was quite an effort, was it worth it? The gain is now that should I change any of priceís behavior, add new prices, or add extra price dependent behavior; it will be much easier to change. The rest of the application does not know about the use of the state pattern. For the tiny amount of behavior I currently have it is not a big deal. But in a more complex system with a dozen or so price dependent methods this would make a big difference. All these changes were small steps, it seems slow to write it like this, but not once did I have to open the debugger. So the process actually flowed quite quickly.

Latest Forum Posts

PEC which stands for Punjab Examination Commission has conducted the annual board exams of 8th Class in the month of February. All the students of public and private schools from 36 districts of Punjab Province appeared in these grade 8 examination conducted by PEC. PEC usually takes one and hal...

Network With Us

Connect with us via a selection of popular social media, networks and other platforms.