This is the last and biggest in this refactoring series. For more great information about refactoring please pick up Martin Fowler's: Refactoring: Improving the Design of Existing Code
According to fowler, we shouldn’t be using a switch statement on something else’s data, but only when it’s our own data. I would say this is also applicable to if/elseif conditionals as well in some cases.
The relevant code is the Charge property.
public double Charge
{
get
{
double result = 0;
switch (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;
}
}
His first step was to move the logic of getCharge() into the movie conditional. This changes the property to look like this:
public double Charge
{
get
{
return Movie.GetCharge(DaysRented);
}
}
And the code that moved into Movie to look like this:
public double GetCharge(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;
}
Wait, there’s data being used from both objects! Of course fowler is onto that and suggests the reason to move it here is because the data that is most likely to change is the movie types “Regular, New Release, Children’s…” and changes to this will have the least ripple effect if changed now that it exists inside the Movie class.
Now we should do the same thing with frequent renter points so that the movie type is ‘being worried about’ in the same class.
Inside rental:
public int FrequentRenterPoints
{
get
{
return Movie.getFrequentRenterPoints(DaysRented);
}
}
Inside Movie:
public int getFrequentRenterPoints(int DaysRented)
{
if ((PriceCode == Movie.NEW_RELEASE) && DaysRented > 1)
{
return 2;
}
else
{
return 1;
}
}
I’m noticing fowler does a great job of ensuring we don’t leave any method callers stranded by pointing old properties at the new code. Sometimes, you may have to remove something, just be sure you’re going back and updating all references to it to point to the new logic.
Now, we’re ready to use polymorphism, right? We can just have a class for each type of movie… NOPE. Unfortunately, we can’t because our type can change at runtime. Fowler suggests using a state pattern. Basically our PriceCode will be come a Price Object… and we’ll have a Price Object for each of the types of prices.
To introduce the state code, Fowler says we’ll be doing 3 refactorings: “Replace type code with State/Strategy” to move the price behavior into a state, “Move Method” to move the Price property into the price class, and then “Replace conditional with Polymorphism” to eliminate the switch.
He says the first step in moving the type code to a state is to ensure all setting of the state is done through the setter (or property in .Net).
Therefore our constructor:
public Movie(string title, int priceCode)
{
_title = title;
_priceCode = priceCode;
}
Becomes:
public Movie(string title, int priceCode)
{
_title = title;
PriceCode = priceCode;
}
Now we add the new classes:
public abstract class Price
{
public abstract int PriceCode{get;}
}
public class ChildrensPrice : Price
{
public override int PriceCode
{
get
{
return Movie.CHILDRENS;
}
}
}
public class NewReleasePrice : Price
{
public override int PriceCode
{
get { return Movie.NEW_RELEASE; }
}
}
public class RegularPrice : Price
{
public override int PriceCode
{
get { return Movie.REGULAR; }
}
}
Compile and check… everything runs good still. Excellent.
Now we have to change the Movie’s accessors to use the new class.
Before:
private int _priceCode;
public int PriceCode
{
get { return _priceCode; }
set { _priceCode = value; }
}
After:
private Price _priceCode;
public int PriceCode
{
get { return _priceCode.PriceCode; }
set
{
switch (value)
{
case Movie.REGULAR:
_priceCode = new RegularPrice();
break;
case Movie.CHILDRENS:
_priceCode = new ChildrensPrice();
break;
case Movie.NEW_RELEASE:
_priceCode = new NewReleasePrice();
break;
default:
throw new ArgumentException("Incorrect price code");
}
}
}
And now we can apply the “Move Method” to the get charge (Charge Property)
Before:
public double GetCharge(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;
}
After:
public double GetCharge(int daysRented)
{
return _priceCode.GetCharge(daysRented);
}
Inside the Price Class:
public abstract class Price
{
public abstract int PriceCode{get;}
public double GetCharge(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;
}
}
(WHEW! and there's more)
So, the first thing we do is change the method to virtual:
public virtual double GetCharge(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;
}
After that we each leg of the case statement as an overriding method inside the sub classes.
public class RegularPrice : Price
{
public override int PriceCode
{
get { return Movie.REGULAR; }
}
public override double GetCharge(int daysRented)
{
double result = 2;
if (daysRented > 2)
{
result += (daysRented - 2) * 1.5;
}
return result;
}
}
public class NewReleasePrice : Price
{
public override int PriceCode
{
get { return Movie.NEW_RELEASE; }
}
public override double GetCharge(int daysRented)
{
return daysRented * 3;
}
}
public class ChildrensPrice : Price
{
public override int PriceCode
{
get
{
return Movie.CHILDRENS;
}
}
public override double GetCharge(int daysRented)
{
double result = 1.5;
if (daysRented > 3)
{
result += (daysRented - 3) * 1.5;
}
return result;
}
}
Once all that is done, we can change the Price.GetCharge method to abstract since there’s nothing left.
public abstract class Price
{
public abstract int PriceCode{get;}
public abstract double GetCharge(int daysRented);
}
Now we do the same thing with FrequentRenterPoints:
public int getFrequentRenterPoints(int _daysRented)
{
if ((PriceCode == Movie.NEW_RELEASE) && _daysRented > 1)
{
return 2;
}
else
{
return 1;
}
}
public abstract class Price
{
public abstract int PriceCode{get;}
public abstract double GetCharge(int daysRented);
public virtual int getFrequentRenterPoints(int _daysRented)
{
if ((PriceCode == Movie.NEW_RELEASE) && _daysRented > 1)
{
return 2;
}
else
{
return 1;
}
}
}
And Movie is like this:
public int getFrequentRenterPoints(int _daysRented)
{
return _priceCode.getFrequentRenterPoints(_daysRented);
}
Then we actually don’t move it to abstract, but change the virtual method to return 1 and then modify the NewReleasePrice as such:
public abstract class Price
{
public abstract int PriceCode{get;}
public abstract double GetCharge(int daysRented);
public virtual int getFrequentRenterPoints(int daysRented)
{
return 1;
}
}
public class NewReleasePrice : Price
{
public override int PriceCode
{
get { return Movie.NEW_RELEASE; }
}
public override double GetCharge(int daysRented)
{
return daysRented * 3;
}
public override int getFrequentRenterPoints(int daysRented)
{
//if > 1 then 2, otherwise return 1
return (daysRented > 1) ? 2 : 1;
}
}
Here's the code thusfar: (sorry i didn't do a 'before')
(CLICK)