A (Very) Simple Example

1
A (Very) Simple Example

• Consolidate duplicate conditional fragments

if (isSpecialDeal()) total price 0.95
send () else total price 0.98
send ()
if (isSpecialDeal()) total price
0.95 else total price 0.98 send ()
2

• Software Engineering II
• Lecture 42
• Fakhar Lodhi

3
Recap
4
A (Very) Simple Example

• Better Still

if (isSpecialDeal()) factor 0.95 else
factor 0.98 total price factor send ()
if (isSpecialDeal()) total price
0.95 else total price 0.98 send ()
5
Refactoring Where to Start?

• How do you identify code that needs to be
  refactored?
• Bad Smells in Code

6
Bad Smells in Code

• Duplicated Code
• bad because if you modify one instance of
  duplicated code but not the others, you (may)
  have introduced a bug!
• Long Method
• long methods are more difficult to understand
  performance concerns with respect to lots of
  short methods are largely obsolete

7
Bad Smells in Code

• Large Class
• Long Parameter List
• Divergent Change
• Shotgun Surgery
• Feature Envy
• Data Clumps
• Primitive Obsession

8
Bad Smells in Code

• Switch Statements
• Parallel Inheritance Hierarchies
• Lazy Class
• Speculative Generality
• Temporary Field
• Message Chains
• Middle Man

9
Bad Smells in Code

• Inappropriate Intimacy
• Alternative Classes with Different Interfaces
• Incomplete Library Class
• Data Class
• Refused Bequest
• Comments (!)

10
Refactoring Example

• A simple program for a video store
• Movie
• Rental
• Customer
• Program is told which movies a customer rented
  and for how long and then, it calculates
• The charges
• Frequent renter points
• Customer object can also print a statement (in
  ASCII)

11
Initial Class Diagram

• statement() works by looping through all rentals
• For each rental, it retrieves its movie and the
  number of days it was rented it also retrieves
  the price code of the movie.
• It then calculates the price for each movie
  rental and the number of frequent renter points
  and returns the generated statement as a string

12
public class Movie public static final int
CHILDREN 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 String getTitle() return
_title public void setPriceCode(int priceCode)
_priceCode priceCode
13
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
14
class Customer private String _name private
Vector _rentals new Vector() public
Customer(String name) _name name public
String getName() return _name public void
addRental (Rental arg) _rentals.addElement(arg)
public String statement()
15
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
16
switch (each.getMovie().getPriceCode())
case Movie.REGULAR thisAmount 2 if
(each.getDaysRented() gt 2) thisAmount
(each.getDaysRented() - 2) 1.5 break
case Movie.NEW_RELAESE thisAmount
each.getDaysRented() 3 break case
Movie.CHILDREN thisAmount 1.5 if
(each.getDaysRented() gt 3) thisAmount
(each.getDaysRented() - 3) 1.5 break
17
// add frequent renter points
frequentRenterPoints // add bonus for two
day new release rental if ((each.getMovie().g
etPriceCode() Movie.NEW_RELEASE)
each.getDaysRented() gt 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(frequentRenterPoin
ts) \n return result
18
How to start?

• We want to decompose statement() into smaller
  pieces which are easier to manage and move
  around.
• Well start with Extract Method
• target the switch statement first

19
Extract Method

• You have a code fragment that can be grouped
  together
• Turn the fragment into a method whose name
  explains the purpose of the fragment

20
switch (each.getMovie().getPriceCode())
case Movie.REGULAR thisAmount 2 if
(each.getDaysRented() gt 2) thisAmount
(each.getDaysRented() - 2) 1.5 break
case Movie.NEW_RELAESE thisAmount
each.getDaysRented() 3 break case
Movie.CHILDREN thisAmount 1.5 if
(each.getDaysRented() gt 3) thisAmount
(each.getDaysRented() - 3) 1.5 break
so each is a parameter and value is returned to
thisAmount

• non-modifiable variables can be passed as
  parameter to new method (if required)
• modified variables require more care since there
  is only one (i.e. thisAmount), we can make it the
  return value of the new method.

each does not change but thisAmount does
Watch for local variables each and thisAmount
Extract method
21
variable each is passed as a parameter
private double amountFor(Rental each) double
thisAmount 0 switch (each.getMovie().getPriceCo
de()) case Movie.REGULAR thisAmount
2 if (each.getDaysRented() gt 2)
thisAmount (each.getDaysRented() - 2) 1.5
break case Movie.NEW_RELAESE thisAmount
each.getDaysRented() 3 break case
Movie.CHILDREN thisAmount 1.5 if
(each.getDaysRented() gt 3) thisAmount
(each.getDaysRented() - 3) 1.5
break return thisAmount
Switch statement is extracted and put in a new
method amountFor()
value of thisAmount is returned
22
Be careful!

• Pitfalls
• be careful about return types in the original
  statement(), thisAmount is a double, but it would
  be easy to make a mistake of having the new
  method return an int. This will cause
  rounding-off error.
• Always remember to test after each change.

23
look at the variable names and use more suitable
ones
private double amountFor(Rental each) double
thisAmount 0 switch (each.getMovie().getPriceCo
de()) case Movie.REGULAR thisAmount
2 if (each.getDaysRented() gt 2)
thisAmount (each.getDaysRented() - 2) 1.5
break case Movie.NEW_RELAESE thisAmount
each.getDaysRented() 3 break case
Movie.CHILDREN thisAmount 1.5 if
(each.getDaysRented() gt 3) thisAmount
(each.getDaysRented() - 3) 1.5
break return thisAmount
24
each and thisAmount
private double amountFor(Rental each) double
thisAmount 0 switch (each.getMovie().getPriceCo
de()) case Movie.REGULAR thisAmount
2 if (each.getDaysRented() gt 2)
thisAmount (each.getDaysRented() - 2) 1.5
break case Movie.NEW_RELAESE thisAmount
each.getDaysRented() 3 break case
Movie.CHILDREN thisAmount 1.5 if
(each.getDaysRented() gt 3) thisAmount
(each.getDaysRented() - 3) 1.5
break return thisAmount
25
each ? aRental thisAmount ? result
private double amountFor(Rental aRental) double
result 0 switch (aRental.getMovie().getPriceCod
e()) case Movie.REGULAR result 2
if (aRental.getDaysRented() gt 2) result
(aRental.getDaysRented() - 2) 1.5 break
case Movie.NEW_RELAESE result
aRental.getDaysRented() 3 break case
Movie.CHILDREN result 1.5 if
(aRental.getDaysRented() gt 3) result
(aRental.getDaysRented() - 3) 1.5
break return result
26
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) // the switch statement has
been replaced by this // call to amountFor
the rest continues as before
27
private double amountFor(Rental aRental) double
result 0 switch (aRental.getMovie().getPriceCod
e()) case Movie.REGULAR result 2
if (aRental.getDaysRented() gt 2) result
(aRental.getDaysRented() - 2) 1.5 break
case Movie.NEW_RELAESE result
aRental.getDaysRented() 3 break case
Movie.CHILDREN result 1.5 if
(aRental.getDaysRented() gt 3) result
(aRental.getDaysRented() - 3) 1.5
break return result
amountFor() uses information from the Rental
class, but it does not use information from the
Customer class where it is currently located.
amountFor() should be part of the Rental class
and not the customer class, so it should be moved
to the Rental class
28
Move method

• Methods should be located close to the data they
  operate.
• we can get rid of the parameters this way.
• lets also rename the method to getCharge() so as
  to clarify what it is doing.
• as a result, back in customer, we must delete the
  old method and change the call from
  amountFor(each) to each.getCharge()
• compile and test

29
private double amountFor(Rental aRental) return
aRental.getCharge()

• initially replace the body with the call

• remove this method later on and call directly

while (rentals.hasMoreElements()) double
thisAmount 0 Rental each (Rental)
rentals.nextElement() thisAmount
each.getCharge() the rest continues as
before
Call to amountFor has been replaced and
getCharge() is called directly
30
New Class Diagram

• No major changes however, Customer is now a
  smaller class and an operation has been moved to
  the class that has the data it needs to do the job