Refactoring
Exercise: The Video Store Example
This exercise is based on the "Video Store Example" described in
Chapter 1 of the book: M. Fowler, Refactoring: Improving the Design of
Existing Code, Addison-Wesley Professional, 1999.
The original code presented in the book is written in Java; the
exercise has
been ported to C++. The code has been built and executed on Scientific
Linux 6 with gcc 4.9.3, Mac OS 10.11 (El Capitan) with Xcode
7.2.1 and GoogleTest 1.7.0, and CentOS 7 with gcc 4.8.5 and GoogleTest 1.6.0.
The
exercise is organized across 15 steps; it is documented in a UML model produced with Enterprise
Architect.
Getting
started
Download a copy of
the exercise source code to your account in naf-school01.desy.de or naf-school02.desy.de; you can browse the source code in GitHub any time.
wget https://github.com/mariagraziapia/VideoStoreAPC/archive/v2.0.tar.gz
Unzip/untar the exercise code release
gunzip v2.0.tar.gz
tar -xvf v2.0.tar
Go into the exercise code directory; you will find 16 directories,
which correspond to the steps of the refactoring exercise.
cd VideoStoreAPC-v2.0

If you wish to reproduce the exercise in your own computing environment, you should first install GoogleTest and define an
environment variable corresponding to the path where you installed it:
setenv GTESTPATH
$HOME/Documents/g4dev/googletest/gtest_install
The original code to be refactored
The original code for the exercise is in the original/ directory. The corresponding class diagram is

and the corresponding sequence diagram, which illustrates the behaviour
of the code before any refactoring, is:

From the Refactoring book:
"What are your impressions about the design of this program? I would
describe it as not well designed
and certainly not object oriented.
For
a simple program like this, that does not really matter. There's
nothing wrong with a quick and dirty simple program. But if this is a
representative fragment of a more complex system, then I have some real
problems with this program. That long routine in the Customer class
does far too much. Many of the things that it does should really be
done by the other classes.
Even so the program works. Is this not just an aesthetic judgment, a
dislike of ugly code? It is until we want to change the system. The
compiler doesn't care whether the code is ugly or clean. But when we
change the system, there is a human involved, and humans do care. A
poorly designed system is hard to change. Hard because it is hard to
figure out where the changes are needed. If it is hard to figure out
what to change, there is a strong chance that the programmer will make
a mistake and introduce bugs.
In this case we have a change that the users would like to make. First
they want a statement printed in HTML
so that the statement can be Web
enabled and fully buzzword compliant. Consider the impact this change
would have. As you look at the code you can see that it is impossible
to reuse any of the behavior of the current statement method for an
HTML statement. Your only recourse is to write a whole new method that
duplicates much of the behavior of statement. Now, of course, this is
not too onerous. You can just copy the statement method and make
whatever changes you need.
But what happens when the charging rules change? You have to fix both
statement and htmlStatement and ensure the fixes are consistent. The
problem with copying and pasting code comes when you have to change it
later. If you are writing a program that you don't expect to change,
then cut and paste is fine. If the program is long lived and likely to
change, then cut and paste is a menace.
This brings me to a second change. The users want to make changes to
the way they classify movies, but they haven't yet decided on the
change they are going to make. They have a number of changes in mind.
These changes will affect both the way renters are charged for movies
and the way that frequent renter points are calculated. As an
experienced developer you are sure that whatever scheme users come up
with, the only guarantee you're going to have is that they will change
it again within six months.
The statement method is where the changes have to be made to deal with
changes in classification and charging rules. If, however, we copy the
statement to an HTML statement, we need to ensure that any changes are
completely consistent. Furthermore, as the rules grow in complexity
it's going to be harder to figure out where to make the changes and
harder to make them without making a mistake.
You may be tempted to make the fewest possible changes to the program;
after all, it works fine. Remember the old engineering adage: "if it
ain't broke, don't fix it." The program may not be broken, but it does
hurt. It is making your life more difficult because you find it hard to
make the changes your users want. This is where refactoring comes in.
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."
Step 0: Tests
From the Refactoring book:
"Whenever one does refactoring, the first step is always the same: one
needs a solid set of tests for
that section of code. The tests are essential because, even though one
pays attention to avoid most of the opportunities for introducing bugs,
we are still human and still make mistakes.
An important part of the tests developed for refactoring is the way
they report their results. They either say "OK" (in this exercise:
meaning that all the strings are identical to the reference strings),
or they print a list of failures (in this exercise: lines that turned
out differently). The tests are thus self-checking.
It is vital to make tests self-checking. If you don't, you end up
spending time hand checking results, and that slows you down."
A simple unit test is available in testVideostore.cc; it is based on GoogleTest. As part of the exercise, you may want to
extend it, adding a few more test cases, reports on failures etc.
Build the test testVideoStore.cc:
cd original
make
make clean (to remove *.o and executable)
The test is automatically run when it is built.
If you wish to run the test again:
./testVideoStore
The output should look like
[==========] Running 7 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 7 tests from VideoStoreTest
[ RUN ] VideoStoreTest.testgetPriceCode
[ OK ] VideoStoreTest.testgetPriceCode (0 ms)
[ RUN ] VideoStoreTest.testsetPriceCode
[ OK ] VideoStoreTest.testsetPriceCode (0 ms)
[ RUN ] VideoStoreTest.testgetTitle
[ OK ] VideoStoreTest.testgetTitle (0 ms)
[ RUN ] VideoStoreTest.testgetDaysRented
[ OK ] VideoStoreTest.testgetDaysRented (0 ms)
[ RUN ] VideoStoreTest.testgetMovie
[ OK ] VideoStoreTest.testgetMovie (0 ms)
[ RUN ] VideoStoreTest.testgetName
[ OK ] VideoStoreTest.testgetName (0 ms)
[ RUN ] VideoStoreTest.teststatement
[ OK ] VideoStoreTest.teststatement (0 ms)
[----------] 7 tests from VideoStoreTest (0 ms total)
[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (0 ms total)
[ PASSED ] 7 tests.
Step 1: Decomposing and Redistributing the Statement Method
Start in the original/ directory:
cd original/
From the Refactoring book:
"The obvious first target of attention is the long statement method: a long method
is a bad
smell
in the code. When we notice a long method like that, we want to
decompose it into smaller pieces. Smaller pieces of code tend to make
things more manageable. They are easier to work with and move around.
The first phase of refactorings in this exercise shows how to split up
the long method and move the pieces to better classes.
Our first step is to find a logical clump of code and use Extract Method.
An obvious piece here is the switch
statement. This looks like it would make a good chunk to extract into
its own method.
When we extract a method, as in any refactoring, we need to know what
can go wrong. If we do the extraction badly, we could introduce a bug
into the program. So before we do the refactoring, we should figure out
how to do it safely.
First we need to look in the fragment for any variables that are local
in scope to the method we are looking at, the local variables and
parameters. This segment of code uses two: each and thisAmount. Of these each is not modified by the code,
but thisAmount
is modified. Any non-modified variable we can pass in as a parameter.
Modified variables need more care. If there is only one, we can return
it. The temp is initialized to 0 each time around the loop and is not
altered until the switch gets to it. So we can just assign the result."
First try to do the refactoring yourself; then you may want to
look at
the solution in step1/. The
corresponding UML diagram is:

Step 2: Renaming Variables
Start in the step1/ directory:
cd step1/
make (build the unit test before refactoring)
From the book:
"I don't like some of the variable names in amountFor, and this is a good place
to change them.
Once I've done the renaming, I compile and test to ensure I haven't
broken anything.
Is renaming worth the effort? Absolutely. Good code should communicate
what it is doing clearly, and variable names are a key to clear code.
Never be afraid to change the names of things to improve clarity. With
good find and replace tools, it is usually not difficult. Strong typing
and testing will highlight anything you miss. Remember"
Any fool can write code that a
computer can understand. Good programmers write code that humans can
understand.
Code that communicates its purpose is very important. I often refactor
just when I'm reading some code. That way as I gain understanding about
the program, I embed that understanding into the code for later so I
don't forget what I learned."
Try to do the renaming yourself. When
you compile the modified code, the unit test is automatically executed
and will tell you whether your modifications altered the expected
behaviour of the code.
The design is unchanged; the cleaner code is in the step2/ directory.

Step 3: Move Method
Start in the step2/ directory:
cd step2/
make (build the unit test before refactoring)
From the Refactoring book:
"amountFor uses information
from the rental, but it does not use information from the customer.
This is a smell of Feature Envy.
This immediately raises our suspicions that the method is on the wrong
object. In most cases a method should be on the object whose data it
uses, thus the method should be moved to the rental. To do this we use Move Method. With this you first
copy the code over to the rental, adjust it to fit in its new home, and
compile.
In this case fitting into its home means removing the parameter.
We also rename the method (getCharge)
as we do the move.
We can now test to see whether the method works. To do this we replace
the body of Customer::amountFor
to delegate to the new method.
// Customer.cpp
...
double Customer::amountFor( const Rental& aRental ) const
{
return aRental.getCharge();
}
We can now compile and test to see whether we have broken anything.
The next step is to find every reference to the old method and adjust
the reference to use the new method. In this case this step is easy
because 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 we have made the change,
the next thing is to remove the old method. The compiler should tell us
whether we missed anything. We then test to see if we have broken
anything.
Sometimes it may be convenient to leave the old method to delegate to
the new method. This is useful if it is a public method and we don't
want to change the interface of the other class.
There is certainly some more we would like to do to Rental::getCharge, but we will
leave it for the moment and return to Customer::statement."
First try to do the
refactoring yourself with the help of the UML diagram below;
then you may want to
look at the solution in step3/.

Step 4: Replace Temp with Query
Start in the step3/ directory:
cd step3/
make (build the unit test before refactoring)
From the Refactoring book:
"thisAmount is now redundant.
It is set to the result of each.getCharge
and not changed afterward. Thus we can eliminate thisAmount by using Replace Temp with Query.
Once we have made that change, we compile and test to make sure we have
not broken anything.
M. Fowler states in the book that he likes to get rid of temporary
variables such as this as much
as possible. Temps are often a problem in that they cause a lot of
parameters to be passed around when they don't have to be. You can
easily lose track of what they are there for. They are particularly
insidious in long methods. Of course, there is a 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 factored."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at the solution in step4/.

Step 5: Extracting Frequent Renter Points
Start in the step4/ directory:
cd step4/
make (build the unit test before refactoring)
From the Refactoring book:
"The next step is to do a similar thing for the frequent renter points.
The rules vary with the DVD, although there is less variation than with
charging. It seems reasonable to put the responsability on the rental.
First we need to use Extract Method
on the frequent renter points part of the code.
Again we look at the use of locally scoped variables. Again each is
used and 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 does not
read the value, however, so we don't need to pass it in as a parameter
as longas we use an appending assignment.
We do the extraction, compile, test and then do a move and compile and
test again. With refactoring, small steps are the best; that way less
tends to go wrong."
First try to do the refactoring yourself with the help of the
UML diagrams below; then you may want to look at
the the solution in step5/.


Step 6: Removing Temps
Start in the step5/ directory:
cd step5/
make (build the unit test before refactoring)
From the Refactoring book:
"We said previously that temporary variables can be a problem.
They are useful only within their own scope, and thus they encourage
long, complex functions. 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 require these totals.
We use Replace Temp with Query to
replace totalAmount and frequentRenterPoints
with query methods. Queries are accessible to any method in the class
and thus encourage a cleaner design without long, complex methods.
We begin by replacing totalAmount with
a charge method on customer. totalAmount was assigned to within
the loop, so we have to copy the loop into the query method."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step6/.

Step 7: Still about removing temps
Start in the step6/ directory:
cd step6/
make (build the unit test before refactoring)
From the Refactoring book:
"After compiling and testing that refactoring, we do the same for frequentRenterPoints.
It is worth stopping to think a bit about the last refactoring. Most
refactorings reduce the amount of code, but this one increases it.
That's because C++ requires a lot of statements to set up a summing
loop. Even a simple summing loop with one line of code per element
needs 4-6 lines of support around it. It is an idiom that is obvious to
any programmer,but is a lot of lines all the same.
The other concern with this refactoring lies in performance. The old
code executed the for
loop once, the new code executes it three times. A for loop that takes
a long time might impair performance. Many programmers would not do
this refactoring simply for this reason. But note the words if and
might. Until we profile we cannot tell how much time is needed for the
loop to calculate or whether the loop is called often enough for it to
affect the overall performance of the system. Don't worry about this
while refactoring. When you optimize, you will have to worry about it,
but you will be in a much better position to do something about it, and
you will have more options to optimize effectively.
These queries are now available for any code written in the customer
class. 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 have 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 mantain."
First try to do the refactoring yourself with the help of the
UML diagrams below; then you may want to look at
the the solution in step7/.


Step 8: Adding new functionality
Start in the step7/ directory:
cd step7/
make (build the unit test before refactoring)
From the Refactoring book:
"We are now at the point where we can take off our refactoring hat and
put on our developer's hat. We can write htmlStatement as follows and add
appropriate tests.
By extracting the calculations we can create the htmlStatement method and reuse all
of the calculation code that was in the original statement
method. We did not copy and paste, so if the calculation rules change,
we have only one place in the code to go. Any other kind of statement
will be really quick and easy to prepare. The refactoring did not take
long. We spent most of the time figuring out what the code did, and we
would have had to do that anyway.
Some code is copied from the ASCII version, mainly due to setting up
the loop. Further refactoring could clean that up. Extracting methods
for header, footer and detail line are one route we could take. You can
see how to do this in the example for Form
Template Method.
But now the users are clamoring again. They are getting ready to change
the classification of the movies in the store. It's still not clear
what changes they want to make, but it sounds like new classifications
will be introduced, and the existing ones could well be changed. The
charges and frequent renter point allocations for these classifications
are to be decided. At the moment, making these kind of changes is
awkward. We have to get into the charge and frequent renter point
methods and alter the conditional code to make changes to film
classifications. Back on with the refactoring hat."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step8/.

Step 9: Replacing the Conditional Logic on Price Code with
Polymorphism
Start in the step8/ directory:
cd step8/
make (build the unit test before refactoring)
From the Refactoring book:
"The first part of this problem is that switch
statement. 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 getCharge
should move onto movie.
For this to work we have to pass in the length of the rental, which, of
course, is data from the rental. The method effectively uses two pieces
of data, the length of the rental and the type of the movie. Why do we
prefer to pass the length of the rental to the movie rather than the
movie type to the rental? It is because the proposed changes are all
about adding new types. Type information generally tends to be more
volatile. If we change the movie type, we want the least ripple effect,
so we prefer to calculate the charge within the movie.
We compile the method into movie and then change the getCharge on rental to use the new
method."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step9.

Step 10: Still about replacing the Conditional Logic with
Polymorphism
Start in the step9/ directory:
cd step9/
make (build the unit test before refactoring)
From the Refactoring book:
"Once we have moved the getCharge
method, we do the same with the frequent renter point calculation. That
keeps both things that vary with the type together on the class that
has the type."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step10/.

Step 11: At last... Inheritance
Start in the step10/ directory:
cd step10/
make (build the unit test before refactoring)
From the Refactoring book:
"We have several types of movie that have different ways of answering
the same question. This sounds like a job for subclasses. We can have
three subclasses of movie, each of which can have its own version of
charge.
This allows us to replace the switch
statement by using polymorphism.
Sadly it has one slight flaw - it doesn't work. A movie can change its
classification during its lifetime. An object cannot change its class
during its lifetime. There is a solution, however, the State pattern.
By adding the indirection we can do the subclassing from the price code
object and change the price whenever we need to.
If you are familiar with the Gang of Four
patterns,
you may wonder, "Is this a state, or is it a strategy?" Does the
price class represent an algorithm for calculating the price (in which
case one may prefer to call it the Pricer or PricingStrategy), or does
it represent a state of the movie (Star Trek X is a new release). At
this stage the choice of pattern (and name) reflects how you want to
think about the structure. At the moment we are thinking about this as
state of movie. If we later decide a strategy communicates our
intention
better, we will refactor to do this by changing the names.
To introduce the state pattern we will use three refactorings. First we
move the type code behavior into the state pattern with Replace
Type Code with State/Strategy. Then we can use Move Method to move the switch
statement into the price class. Finally we use Replace
Conditional with Polymorphism to eliminate the switch statement.
We begin with Replace
Type Code with State/Strategy. This first step is to use Self
Encapsulate Field on
the type code to ensure that all uses of the type code go through
getting and setting methods. Because most of the code came from the
other classes, most methods already use the getting method. However,
the constructors do access the price code. We can use the setting
method instead."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step11/.

Step 12: Adding new classes
Start in the step11/ directory:
cd step11/
make (build the unit test before refactoring)
From the Refactoring book:
"Now we add the new classes. We provide the type code behavior in the
price object. We do this with an abstract
method on price and concrete
methods in the subclasses.
We can compile the new classes at this point.
Now we need to change the movie's accessors for the price code to use
the new class."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step12/.

Step 13: Move Method
Start in the step12/ directory:
cd step12/
make (build the unit test before refactoring)
From the Refactoring book:
"Now we apply Move Method to getCharge."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step13/.

Step 14: Replace Conditional with Polymorphism
Start in the step13/ directory:
cd step13/
make (build the unit test before refactoring)
From the Refactoring book:
"Once it is moved, we can start using Replace
Conditional with Polymorphism.
We do this by taking one leg of the case statement at time and creating
an overriding method. We start with RegularPrice.
This overrides the parent case statement, which we just leave as it is.
We compile and test for this case, then take the next leg, compile and
test.
When we have done that with all the legs, we make Price::getCharge a pure
virtual function."
First try to do the refactoring yourself with the help of the
UML diagram below; then you may want to look at
the the solution in step14/.

Step 15
Start in the step14/ directory:
cd step14/
make (build the unit test before refactoring)
From the Refactoring book:
"We can now apply the same procedure to getFrequentRenterPoints.
In this case however, we do not make the superclass method pure
virtual. Instead, we create an overriding method for the new releases
and leave a defined method (as the default) on the superclass."
First try to do the refactoring yourself with the help of the
UML diagrams below; then you may want to look at
the the solution in step15/.


Putting in the state pattern was quite an effort. Was it worth it? The
gain is that if we change any price's behavior, add new prices, or add
extra price-dependent behavior, the change will be much easier to make.
The rest of the application does not know about the use of the state
pattern. For the tiny amout of behavior we currently have, it is not a
big deal. 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 this way, but by performing small
steps and checking them immediately with unit tests, we can do without
opening the debugger, so the process flows quite quickly.
Back to
APC2016 Lectures