Software

Re-Engineering
WEEK 5
Refactoring, A First
Example
The Starting Point
Refactoring a simple program
Calculate and print a statement of a customer's charges at a video
store.
The program is told which movies a customer rented and for how long.
It then calculates the charges, which depend on how long the movie is
rented, and identifies the type of the movie.
◦ regular, children's, or new releases.

the statement also computes frequent renter points, which vary

depending on whether the film is a new release.
Class diagram of the starting-point classes
Only the most important features are shown
public class Movie {
public static final int CHILDRENS = 2;
public static final int REGULAR = 0;
public static final int NEW_RELEASE = 1;
private String _title;
private int _priceCode;
public Movie(String title, int priceCode) {
_title = title;
_priceCode = priceCode;
}
public int getPriceCode() {
return _priceCode;
}
public void setPriceCode(int arg) {
_priceCode = arg;
}
public String getTitle (){
return _title;
}
}
 Rental
class Rental {

private Movie _movie;

private int _daysRented;

public Rental(Movie movie, int daysRented) {

_movie = movie;
_daysRented = daysRented;
}

public int getDaysRented() {

return _daysRented;
}

public Movie getMovie() {

return _movie;
}

}
 Customer
class Customer {

private String _name;

private Vector _rentals = new Vector();

public Customer (String name){

_name = name;
}

public void addRental(Rental arg) {

_rentals.addElement(arg);
}

public String getName (){

return _name;
}
Interactions for the statement method
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();

//determine amounts for each line

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.CHILDRENS:
thisAmount += 1.5;
if (each.getDaysRented() > 3)
thisAmount += (each.getDaysRented() - 3) * 1.5;
break; }
 // add frequent renter points
frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}

//add footer lines

result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
Comments on the Starting
Program
Not well designed, not OO
Statement method does too much work
Hard to change
Possible change: Print a statement in HTML
◦ Impossible to reuse statement method

Possible change: change charging rules

◦ Both statement and htmlStatement change

Possible change: change movie classification

◦ Change in charge calculation and frequent renter point calculation
 Tip

When you find you have to add a feature to a

program, and the program's code is not structured in
a convenient way to add the feature, first refactor the
program to make it easy to add the feature, then add
the feature.
 The First Step in Refactoring
Tip
Before you start refactoring, check that you have a
solid suite of tests. These tests must be self-checking.
Decomposing and Redistributing
the Statement Method
Smaller pieces of code are more manageable
Aim: to write an HTML statement method with minimum duplication of
code
Step 1: use extract method refactoring
◦ Switch statement
◦ Look out for local variables
◦ Non modified local variables can be function parameters
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();

//determine amounts for each line

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.CHILDRENS:
thisAmount += 1.5;
if (each.getDaysRented() > 3)
thisAmount += (each.getDaysRented() - 3) * 1.5;
break; }
 // add frequent renter points
frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}

//add footer lines

result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();

thisAmount = amountFor(each);

// add frequent renter points

frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
 //add footer lines
result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
}
Private int amountFor(Rental each) {
int 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.CHILDRENS:
thisAmount += 1.5;
if (each.getDaysRented() > 3)
thisAmount += (each.getDaysRented() - 3) * 1.5;
break;
}
return thisAmount;
}
 The tests blew up.
Return type should be double and not int.

Private double amountFor(Rental each) {

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.CHILDRENS:
thisAmount += 1.5;
if (each.getDaysRented() > 3)
thisAmount += (each.getDaysRented() - 3) * 1.5;
break;
}
return thisAmount;
}
 Tip

Refactoring changes the programs in small steps. If you

make a mistake, it is easy to find the bug.
 Changing some names
Private double amountFor(Rental aRental) {
double result= 0;
switch (aRental.getMovie().getPriceCode()) {
case Movie.REGULAR:
result += 2;
if (aRental.getDaysRented() > 2)
result += (aRental.getDaysRented() - 2) * 1.5;
break;
case Movie.NEW_RELEASE:
result += aRental.getDaysRented() * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (aRental.getDaysRented() > 3)
result += (aRental.getDaysRented() - 3) * 1.5;
break;
}
return result;
}
 Tip

Any fool can write code that a computer can

understand. Good programmers write code that
humans can understand.
 Moving the Amount Calculation
amountFor uses information from the rental, not from customer
Class Customer…
Private double amountFor(Rental aRental) {
double result= 0;
switch (aRental.getMovie().getPriceCode()) {
case Movie.REGULAR:
result += 2;
if (aRental.getDaysRented() > 2)
result += (aRental.getDaysRented() - 2) * 1.5;
break;
case Movie.NEW_RELEASE:
result += aRental.getDaysRented() * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (aRental.getDaysRented() > 3)
result += (aRental.getDaysRented() - 3) * 1.5;
break;
}
return result;
}
 Using Move Method refactoring
Method should be on the object whose data it uses

Class Rental…
Private double getCharge() {
double result= 0;
switch (getMovie().getPriceCode()) {
case Movie.REGULAR:
result += 2;
if (getDaysRented() > 2)
result += (getDaysRented() - 2) * 1.5;
break;
case Movie.NEW_RELEASE:
result += getDaysRented() * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (getDaysRented() > 3)
result += (getDaysRented() - 3) * 1.5;
break;
}
return result;
}
 Customer.amountFor delegates to the new method

class Customer...
private double amountFor(Rental aRental) {
return aRental.getCharge();
}

Next step: find every reference to the old method and

adjust the reference to use the new method
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();

thisAmount = amountFor(each);

// add frequent renter points

frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
 //add footer lines
result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
}
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();

thisAmount = each.getCharge();

// add frequent renter points

frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
 //add footer lines
result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
}
State of classes after moving the charge method
 thisAmount is now redundant
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
Rental each = (Rental) rentals.nextElement();

// add frequent renter points

frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(each.getCharge()) + "\n";
totalAmount += each.getCharge();
}
 //add footer lines
result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
}

• Get rid of temporary variables as much as possible

 Extracting Frequent Renter Points
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();

// add frequent renter points

frequentRenterPoints ++;

// add bonus for a two day new release rental

if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& each.getDaysRented() > 1) frequentRenterPoints ++;

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(each.getCharge()) + "\n";
totalAmount += each.getCharge();
}
 //add footer lines
result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
}

• Again look at the local variables

• each is used
• can be passed in as a parameter
• frequentRenterPoints have a value beforehand
• no need to pass it in as a parameter
Class Customer…
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();
frequentRenterPoints += each.getFrequentRenterPoints();

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(each.getCharge()) + "\n";
totalAmount += each.getCharge();
}

//add footer lines

result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
Class Rental…
int getFrequentRenterPointes() {
if ((getMovie().getPriceCode() == Movie.NEW_RELEASE)
&& getDaysRented() > 1)
return 2;
else
return 1;
}
Class diagram before extraction and movement of the
frequent renter points calculation
Sequence diagrams before extraction and movement of the
frequent renter points calculation
Class diagram after extraction and movement of the frequent
renter points calculation
Sequence diagram after extraction and movement of the
frequent renter points calculation
Removing Temps
Temporary variables can be a problem
Encourage long complex routines
Use Replace Temp with Query refactoring
◦ Replace totalAmount and frequentRentalPoints with query methods

Queries are accessible to any methods in the class

◦ Encourage cleaner design without long, complex methods
Class Customer…
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();
frequentRenterPoints += each.getFrequentRenterPoints();

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(each.getCharge()) + "\n";
totalAmount += each.getCharge();
}

//add footer lines

result += "Amount owed is " + String.valueOf(totalAmount) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
Class Customer…
public String statement() {
int frequentRenterPoints = 0;
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
Rental each = (Rental) rentals.nextElement();
frequentRenterPoints += each.getFrequentRenterPoints();

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(each.getCharge()) + "\n";
}

//add footer lines

result += "Amount owed is " + String.valueOf(getTotalCharge()) +"\n";
result += "You earned " + String.valueOf(frequentRenterPoints)+
" frequent renter points";
return result;
}
private double getTotalCharge() {
double result = 0;
Enumeration rentals = _rentals.elements();
while (rentals.hasMoreElements()) {
Rental each = (Rental) rentals.nextElement();
result += each.getCharge();
}
return result;
}

• Not the simplest case of Replace Temp with Query

• totalAmount was assigned to within the loop
• Loop was copied into the query
 Replacing frequentRenterPoints with Query

Class Customer…
public String statement() {
Enumeration rentals = _rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while (rentals.hasMoreElements()) {
Rental each = (Rental) rentals.nextElement();

//show figures for this rental

result += "\t" + each.getMovie().getTitle()+ "\t" +
String.valueOf(each.getCharge()) + "\n";
}

//add footer lines

result += "Amount owed is " + String.valueOf(getTotalCharge()) +"\n";
result += "You earned " + String.valueOf(getTotalFrequentRenterPoints())+
" frequent renter points";
return result;
}
private int getTotalFrequentRenterPoints(){
int result = 0;
Enumeration rentals = _rentals.elements();
while (rentals.hasMoreElements()) {
Rental each = (Rental) rentals.nextElement();
result += each.getFrequentRenterPoints();
}
return result;
}
Class diagram before extraction of the totals
Sequence diagram before extraction of the totals
Class diagram after extraction of the totals
Sequence diagram after extraction of the totals
 Adding htmlStatement method

public String htmlStatement() {

Enumeration rentals = _rentals.elements();
String result = "<H1>Rentals for <EM>" + getName() + "</EM></H1><P>\n";
while (rentals.hasMoreElements()) {
Rental each = (Rental) rentals.nextElement();
//show figures for each rental
result += each.getMovie().getTitle()+ ": " +
String.valueOf(each.getCharge()) + "<BR>\n";
}
//add footer lines
result += "<P>You owe <EM>" + String.valueOf(getTotalCharge()) +"</EM><P>\n";
result += "On this rental you earned <EM>" +
String.valueOf(getTotalFrequentRenterPoints()) +
"</EM> frequent renter points<P>";
return result;
}
Replacing the Conditional Logic on
Price Code with Polymorphism
Possible change
◦ Change the classification of the movies

Charges and frequent renter point allocation for these classifications are
yet to be decided
Tackle the switch statement
◦ Switch statement should be based on your own data, not on someone else’s
Class Rental…
Private double getCharge() {
double result= 0;
switch (getMovie().getPriceCode()) {
case Movie.REGULAR:
result += 2;
if (getDaysRented() > 2)
result += (getDaysRented() - 2) * 1.5;
break;
case Movie.NEW_RELEASE:
result += getDaysRented() * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (getDaysRented() > 3)
result += (getDaysRented() - 3) * 1.5;
break;
}
return result;
}
 getCharge should move onto movie
Class Movie…
double getCharge(int daysRented) {
double result= 0;
switch (getPriceCode()) {
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;
}
Question: Why to prefer to pass the length of rental to the
movie rather than the movie type to the rental?
Answer: Type information generally is more volatile.
class Rental...
double getCharge() {
return _movie.getCharge(_daysRented);
}
int getFrequentRenterPoints() {
return _movie.getFrequentRenterPoints(_daysRented);
}

class Movie...
int getFrequentRenterPoints(int daysRented) {
if ((getPriceCode() == Movie.NEW_RELEASE) && daysRented > 1)
return 2;
else
return 1;
}
Class diagram before moving methods to movie
Class diagram after moving methods to movie
At last … Inheritance

Using inheritance on movie

Inheritance
Allows us to replace the switch statement by using polymorphism
Doesn’t work !!!
◦ Movie can change classification

Use state pattern instead

Using the State pattern on movie
State pattern
Three refactorings needed
◦ Replace Type Code with State/Strategy
◦ Move Method
◦ Replace Conditional with Polymorphism
 Self Encapsulate Field
class Movie...
public Movie(String name, int priceCode) {
_title = name;
_priceCode = priceCode;
}

class Movie
public Movie(String name, int priceCode) {
_title = name;
setPriceCode(priceCode);
}
 Adding new classes
abstract class Price {
abstract int getPriceCode();
}

class ChildrensPrice extends Price {

int getPriceCode() {
return Movie.CHILDRENS;
}
}

class NewReleasePrice extends Price {

int getPriceCode() {
return Movie.NEW_RELEASE;
}
}

class RegularPrice extends Price {

int getPriceCode() {
return Movie.REGULAR;
}
}
Change movie’s accessors
public int getPriceCode() {
return _priceCode;
}

public setPriceCode (int arg) {

_priceCode = arg;
}

private int _priceCode;

• Replace price code field with a price field

• Change the accessors
class Movie...
public int getPriceCode() {
return _price.getPriceCode();
}
public void setPriceCode(int arg) {
switch (arg) {
case REGULAR:
_price = new RegularPrice();
break;
case CHILDRENS:
_price = new ChildrensPrice();
break;
case NEW_RELEASE:
_price = new NewReleasePrice();
break;
default:
throw new IllegalArgumentException("Incorrect Price Code");
}
}

private Price _price;

 Move Method
class Movie...
double getCharge(int daysRented) {
double result = 0;
switch (getPriceCode()) {
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;
}
class Movie...
double getCharge(int daysRented) {
return _price.getCharge(daysRented);
}
Class Price…
double getCharge(int daysRented){
double result = 0;
switch (getPriceCode()) {
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;
}
 Replace Conditional with Polymorphism

class RegularPrice...
double getCharge(int daysRented){
double result = 2;
if (daysRented > 2)
result += (daysRented - 2) * 1.5;
return result;
}
class ChildrensPrice…
double getCharge(int daysRented){
double result = 1.5;
if (daysRented > 3)
result += (daysRented - 3) * 1.5;
return result;
}
class NewReleasePrice...
double getCharge(int daysRented){
return daysRented * 3;
}
class Price...
abstract double getCharge(int daysRented);
 Do the same with getFrequentRenterPoints

class Rental...
int getFrequentRenterPoints(int daysRented) {
if ((getPriceCode() == Movie.NEW_RELEASE) && daysRented > 1)
return 2;
else
return 1;
}
 Move the method over to Price

Class Movie...
int getFrequentRenterPoints(int daysRented) {
return _price.getFrequentRenterPoints(daysRented);
}

Class Price...
int getFrequentRenterPoints(int daysRented) {
if ((getPriceCode() == Movie.NEW_RELEASE) && daysRented > 1)
return 2;
else
return 1;
}
 Override method for new releases

Class NewReleasePrice
int getFrequentRenterPoints(int daysRented) {
return (daysRented > 1) ? 2: 1;
}

Class Price...
int getFrequentRenterPoints(int daysRented){
return 1;
}
Interactions using the state pattern
Class diagram after addition of the state pattern
 Final Thoughts
A simple example
Refactoring lead to better distributed responsibilities and easier to maintain
code
Rhythm of refactoring
◦ Test, small change, test, small change, test, small change