Refactoring, a first example
Martin Fowler
Fowler@acm.org
Refactoring is a technique to better the quality of existing code. It works by applying a series of teentsy stairs, apiece of which changes the internal structure of the code, while maintaining its external behavior. You begin with a program that runs correctly, but is not well organized, refactoring improves its complex body part, making IT easier to maintain and cover.
Currently on that point is not much written on refactoring. I rich person been devising a start at doing something about this with some presentations and other articles. At some point these may evolve into a book. This papers is work in progress, so delight be lenient with its inevitable errors!
When I describe refactoring to people, one of the nigh difficult things to scram terminated is the rhythm of refactoring. This is the fashio you do small step by small ill-use, slowly improving code superior. So information technology seems that the outdo way to deal with this is to give an example. American Samoa soon as I do this, however, I run into a proud problem. If I piece a large program, then describing it and how IT is refactored is too complicated for any reader to work direct. However if I cull out a program that is miniscule sufficiency to be comprehensible, so refactoring does not expression same it is worthwhile.
Thus I�m in the classic bind of anyone who wants to describe techniques that are utilitarian for real world programs. Frankly it is non deserving the effort to do the refactoring that I�m going to show you along a microscopic program suchlike the one I�m leaving to use. But if the code I�m showing you is part of a larger system, then the refactoring presently becomes important. Thus I have to ask you to look at this and imagine in the context of a much larger system.
The Terminus a quo
The sample program is quite simple. IT is a program to photographic print out a statement of a customer�s charges at a video store. There are several classes that represent various video elements. Present�s a class diagram to show them.
- A class plot of the starting point classes. Only the nigh pivotal features are shown. The annotation is UML [Fowler]
Here is the code for the classes. DomainObject is a general class that does a hardly a standard things, such as hold a name.
populace class DomainObject { public DomainObject (Bowed stringed instrument cite) { _name = name; }; public DomainObject () {}; public String key out () { return _name; }; public String out toString() { riposte _name; }; protected String _name = "no gens"; } Movie represents the notion of a film. A video depot might have various tapes in banal of the same picture show
public class Movie extends DomainObject { public static final int CHILDRENS = 2; public static final int REGULAR = 0; state-supported static final int NEW_RELEASE = 1; privy int _priceCode; public Movie(String distinguish, int priceCode) { _name = name; _priceCode = priceCode; } public int priceCode() { retort _priceCode; } semipublic void persist() { Registrar.add ("Movies", this); } state-supported electricity Movie make(String name) { return (Flic) Registrar.get ("Movies", nominate); } } The movie uses a category called a registrar (not shown) as a classify to hold in instances of movie. I as wel do this with other classes. I use the message persist to tell an targe to save itself to the registrar. I posterior then recollect the object, supported its name, with a get(String) method acting.
The tape class represents a physical tape.
class Tape extends DomainObject { public Movie movie() { riposte _movie; } overt Tape(String serialNumber, Movie movie) { _serialNumber = serialNumber; _movie = movie; } private String _serialNumber; private Movie _movie; } The rental sort out represents a customer renting a picture.
class Letting extends DomainObject { public int daysRented() { restoration _daysRented; } public Videotape tape() { return _tape; } private Tape _tape; public Rental(Taping tape, int daysRented) { _tape = tape; _daysRented = daysRented; } private int _daysRented; } The customer class represents the customer. Hitherto all the classes have been inarticulate encapsulated data. Customer holds each the behavior for producing a statement in its financial statement() method acting.
class Client extends DomainObject { public Customer(String name) { _name = name; } public Drawing string statement() { double up totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Lease) rentals.nextElement(); //determine amounts for each job switch (each.tapeline().movie().priceCode()) { case Movie.REGULAR: thisAmount += 2; if (for each one.daysRented() > 2) thisAmount += (each.daysRented() - 2) * 1.5; reveal; case Picture show.NEW_RELEASE: thisAmount += each.daysRented() * 3; break; case Motion picture.CHILDRENS: thisAmount += 1.5; if (each.daysRented() > 3) thisAmount += (each.daysRented() - 3) * 1.5; break; } totalAmount += thisAmount; // tot frequent renter points frequentRenterPoints ++; // ADHD fillip for a 2 day new waiver rental if ((each.tape().moving picture().priceCode() == Pic.NEW_RELEASE) &&ere; each.daysRented() > 1) frequentRenterPoints ++; //picture figures for this holding result += "\t" + for each one.tape().movie().name()+ "\t" + String.valueOf(thisAmount) + "\n"; } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent tenant points"; return result; } overt evacuate addRental(Rental arg) { _rentals.addElement(arg); } public static Client generate(Cosmic string diagnose) { return (Client) Registrar.get("Customers", name); } public void persist() { Registrar.tot("Customers", this); } private Transmitter _rentals = new Vector(); } What are your impressions about the design of this program? I would describe equally not well designed, and certainly non object-homeward. For a needled curriculum is this, that does not really matter. In that location�s nothing wrong with a flying and dirty simple program. Only if we imagine this Eastern Samoa a fragment of a many complex system, so I have some substantial problems with this program. That long statement routine in the Customer does far overly much. Many of the things that it does should actually be done away the other classes.
This is really brought out by a current requirement, just in from the users, they desire a siamese statement in HTML. As you look at the code you can see that it is unachievable to reprocess any of the conduct of the current statement() method for an htmlStatement() . Your only recourse is to write a whole radical method that duplicates much of the behavior of statement(). Now of course this is not too taxing. You can just transcript the financial statement() method and make whatever changes you need. Thus the lack of design does non do overmuch to hamper the writing of htmlStatement(), (although it might be sly to cypher out exactly where to do the changes). Simply what happens when the charging rules variety? You have to fix some statement() and htmlStatement(), and ensure the fixes are consistent. The problem from cut and pasting codification comes when you hold to change it later. Thus if you are piece of writing a program that you don�t expect to change, then cut and paste is fine. If the program is aware lived and probably to change, and so cut and paste is a menace.
But you still have to write the htmlStatement() program. You may feel that you should not reach into the existing command() method, after all IT whole caboodle fine. Call up the old technology adage: "if it ain�t broke, don�t fix it". statement() may not be broke, but it does anguish. It is making your life more difficult to indite the htmlStatement() method.
So this is where refactoring comes in. When you bump you experience to add a feature film to a program, and the program�s codification is not structured in a spacious way to add the feature; and then first refactor the program to pull through easy to add the feature, then add the feature.
Extracting the Amount Calculation
The obvious first target of my attention is the overly long statement() method. When I view a long method equivalent that, I am looking to rent a chunk of the code an extract a method from it.
Extracting a method acting is taking the chunk of code and making a method acting out of it. An overt slice Hera is the change financial statement (which I'm highlight below).
semipublic String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Annuity in advance Record for " + name() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Rental) rentals.nextElement(); //determine amounts for each line switch (each.tape measure().movie().priceCode()) { case Movie.Orderly: thisAmount += 2; if (each.daysRented() > 2) thisAmount += (each.daysRented() - 2) * 1.5; break; case Movie.NEW_RELEASE: thisAmount += to each one.daysRented() * 3; break; case Pic.CHILDRENS: thisAmount += 1.5; if (each.daysRented() > 3) thisAmount += (for each one.daysRented() - 3) * 1.5; break; } totalAmount += thisAmount; // sum frequent renter points frequentRenterPoints ++; // add fillip for a cardinal day new release rental if ((each.tapeline().movie().priceCode() == Movie.NEW_RELEASE) &ere;& each.daysRented() > 1) frequentRenterPoints ++; //show figures for this rental result += "\t" + each.tape().movie().name()+ "\t" + Drawing string.valueOf(thisAmount) + "\n"; } //add footer lines answer += "Number owed is " + String.valueOf(totalAmount) + "\n"; result += "You attained " + Thread.valueOf(frequentRenterPoints) + " buy at tenant points"; return result; } This looks like it would make a good chunk to press out into its ain method. When we extract a method acting, we postulate to look in the fragment for any variables that are topical anesthetic in reach to the method we are look, that topical variables and parameters. This segment of code uses two: each and thisAmount . Of these for each one is not modified by the code but thisAmount is modified. Any non-modified shifting we can pass in as a parameter. Varied variables deman more care. If there is sole peerless we can return it. The temp is initialized to 0 each fourth dimension round the loop, and non altered until the switch gets its manpower on it. Soh we can clean ascribe the result. The extraction looks like this.
national String along financial statement() { double totalAmount = 0; int frequentRenterPoints = 0; Numbering rentals = _rentals.elements(); String result = "Lease Record book for " + name() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental all = (Rental) rentals.nextElement(); //determine amounts for each line thisAmount = amountOf(each); totalAmount += thisAmount; // sum up shop at renter points frequentRenterPoints ++; // add up fillip for a two day new release rental if ((all.tape().movie().priceCode() == Film.NEW_RELEASE) && each.daysRented() > 1) frequentRenterPoints ++; //show up figures for this rental outcome += "\t" + each.record().movie().name()+ "\t" + Chain.valueOf(thisAmount) + "\n"; } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + Drawstring.valueOf(frequentRenterPoints) + " frequent renter points"; paying back result; } semiprivate int amountOf(Renting for each one) { int thisAmount = 0; switch (for each one.tape().movie().priceCode()) { case Movie.REGULAR: thisAmount += 2; if (each.daysRented() > 2) thisAmount += (each.daysRented() - 2) * 1.5; break; case Movie.NEW_RELEASE: thisAmount += each.daysRented() * 3; break; case Motion picture.CHILDRENS: thisAmount += 1.5; if (each.daysRented() > 3) thisAmount += (from each one.daysRented() - 3) * 1.5; break; } return thisAmount; } When I did this the tests blew up. A couple of the test figures gave me the amiss answer. I was puzzled for a couple of seconds then realized what I had done. Unwisely I had made the return type of amountOf() int instead of double .
personal double amountOf(Rental each) { double thisAmount = 0; switch (each.tape().movie().priceCode()) { case Movie.REGULAR: thisAmount += 2; if (each.daysRented() > 2) thisAmount += (each.daysRented() - 2) * 1.5; break; case Movie.NEW_RELEASE: thisAmount += each.daysRented() * 3; break; instance Movie.CHILDRENS: thisAmount += 1.5; if (from each one.daysRented() > 3) thisAmount += (each.daysRented() - 3) * 1.5; break; } return thisAmount; } It�s the sort of silly mistake that I often make, and information technology stern represent a pain to racetrack Down equally Java converts ints to doubles without complaining (only merrily rounding). Fortunately it was easy to discover in this case, because the change was then small. Hera is the essence of the refactoring process illustrated by chance event. Because to each one change is so small, any errors are very easy to find. You don't expend oblong debugging, evening if you are as careless As I am.
This refactoring has taken a big method and disorganised it fallen into two much more manageable chunks. We can directly consider the chunks a bit better. I don't like few of the variables names in amountOf() and this is a good put down to change them.
private double amountOf(Rental aRental) { double result = 0; switch (aRental.tape().moving-picture show().priceCode()) { case Movie.REGULAR: result += 2; if (aRental.daysRented() > 2) result += (aRental.daysRented() - 2) * 1.5; break; case Movie.NEW_RELEASE: result += aRental.daysRented() * 3; break; case Movie.CHILDRENS: result += 1.5; if (aRental.daysRented() > 3) solution += (aRental.daysRented() - 3) * 1.5; break; } return result; }
Is that renaming deserving the travail? Absolutely. Good code should communicate what it is doing clearly, and variable name calling are key to clear inscribe. Ne'er be afraid to exchange the names to things to better lucidity. With salutary find and supersede tools, information technology is commonly not difficult. Efficacious typing and testing will highlight anything you overlook. Remember any fool can write code that a computer can understand, good programmers write cypher that humans can understand.
Moving the measure calculation
As I feel at amountOf, I can see that it uses information from the rental, just does non use information from the customer. This method acting is thus on the wrong object, it should beryllium moved to the rental. To move out a method you first re-create the code over to rental, adjust it to fit in its new rest home and compile.
Sort out Rental double charge() { double result = 0; switch (tape measure().movie().priceCode()) { case Moving-picture show.REGULAR: result += 2; if (daysRented() > 2) result += (daysRented() - 2) * 1.5; break; case Picture show.NEW_RELEASE: result += daysRented() * 3; break; case Movie.CHILDRENS: lead += 1.5; if (daysRented() > 3) solution += (daysRented() - 3) * 1.5; break; } return result; } In this grammatical case proper into its new home means removing the parametric quantity.
The next dance step is to find every reference to the old method, and adjusting the reference to utilize the new method. In this case this step is rich as we just created the method and it is in only one place. In general, nonetheless, you need to do a find across all the classes that might exist using that method.
public Cosmic string statement() { double totalAmount = 0; int frequentRenterPoints = 0; Count rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; spell (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Rental) rentals.nextElement(); //determine amounts for each line thisAmount = each.charge(); totalAmount += thisAmount; // add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.tape().movie().priceCode() == Movie.NEW_RELEASE) &A;& apiece.daysRented() > 1) frequentRenterPoints ++; //show figures for this rental result += "\t" + each.videotape().motion-picture show().name()+ "\t" + String.valueOf(thisAmount) + "\n"; } //add footer lines issue += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } When I've made the convert the future thing is to take out the old method. The compiler should past tell me if I missed anything.
There is certainly some more I would like to do to Holding.charge() but I will leave it for the time being and return to Customer.assertion().
The next thing that strikes me is that thisAmount() is now pretty redundant. It is set to the result of each.charge() and not transformed afterwards. Thus I can eliminate thisAmount by replacing a temp with a query .
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Reckoning rentals = _rentals.elements(); Strand result = "Rental Show for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement(); //determine amounts for each line totalAmount += each.charge(); // add frequent tenant points frequentRenterPoints ++; // add bonus for a two day new release rental if ((for each one.tape().movie().priceCode() == Movie.NEW_RELEASE) && each.daysRented() > 1) frequentRenterPoints ++; //show figures for this rental outcome += "\t" + each.tape().motion picture().name()+ "\t" + Strand.valueOf(each.charge()) + "\n"; } //add pedestrian lines outcome += "Amount owed is " + String.valueOf(totalAmount) + "\n"; upshot += "You attained " + Strand.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } I like to eliminate temporary variables like thus as far as possible. Temps are often a trouble therein they cause a lot of parameters to get passed around when they don't ask to. You can easily lose racecourse of what they are in that respect for. They are particularly insidious in long methods. Of course in that location is a small performance Price to pay, present the blame is now calculated twice. But it is easy to optimize that in the rental class, and you can optimize much more in effect when the encode is properly refactored.
Extracting Regular Renter Points
The next step is to do a similar thing for the frequent renter points. Again the rules deviate with the tape, although in that respect is inferior fluctuation than with the charging. Simply it seems reasonable to put the responsibility on the rental. First we need to extract a method acting from the frequent renter points split up of the code (highlighted downstairs).
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String along effect = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Rental for each one = (Rental) rentals.nextElement(); //determine amounts for each line totalAmount += for each one.charge(); // add shop at renter points frequentRenterPoints ++; // add u bonus for a two daytime new release belongings if ((apiece.tape().movie().priceCode() == Motion picture.NEW_RELEASE) && each.daysRented() > 1) frequentRenterPoints ++; //present figures for this rental result += "\t" + each.tape().movie().name()+ "\t" + String.valueOf(each.charge()) + "\n"; } //ADD footnote lines result += "Amount owed is " + Strand.valueOf(totalAmount) + "\n"; solution += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } Again we deal the use of locally scoped variables. Again it uses apiece , which give notice beryllium passed in Eastern Samoa a parameter. The other temp used is frequentRenterPoints . In this case frequentRenterPoints does have a treasure beforehand. The body of the extracted method doesn't read the value, even so, indeed we don't need to pass it in as a parameter as long American Samoa we employment an appending assignment.
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + call() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement(); //determine amounts for each line totalAmount += for each one.charge(); // add together frequent renter points frequentRenterPoints += frequentRenterPointOf(all); //establish figures for this rental result += "\t" + each.taping().movie().name()+ "\t" + String.valueOf(each.accusation()) + "\n"; } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; event += "You earned " + Strand.valueOf(frequentRenterPoints) + " frequent tenant points"; proceeds result; } int frequentRenterPointOf(Rental each) { if ((each.tape().movie().priceCode() == Picture.NEW_RELEASE) && for each one.daysRented() > 1) return 2; else pass 1; } I did the extraction, compiled and tested, and and so did a move. With refactoring small steps are the best, that way less tends to go wrong.
world String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Numeration rentals = _rentals.elements(); String result = "Rental Record for " + name() + "\n"; while (rentals.hasMoreElements()) { Holding each = (Rental) rentals.nextElement(); //determine amounts for all line totalAmount += apiece.commit(); // add frequent tenant points frequentRenterPoints += each.frequentRenterPoints(); //show figures for this rental resultant role += "\t" + each.tape().picture().name()+ "\t" + String.valueOf(each.charge()) + "\n"; } //tally footer lines result += "Quantity owed is " + String.valueOf(totalAmount) + "\n"; resultant += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } int frequentRenterPoints() { if ((tapeline().movie().priceCode() == Flic.NEW_RELEASE) && daysRented() > 1) income tax return 2; else return 1; } Removing Temps
As I suggested in front, temporary variables ass be a trouble. They are only expedient within their own subprogram, and thus they encourage long Gordian routines. In this casing we take over two temporary variables, some of which are organism 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 course of study, and thus encourage a cleaner design without long complex methods.
I began by replacing totalAmount with a billing() method happening customer.
public String affirmation() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String event = "Material possession Record for " + name() + "\n"; patc (rentals.hasMoreElements()) { Rental to each one = (Rental) rentals.nextElement(); // hyperkinetic syndrome frequent tenant points frequentRenterPoints += each.frequentRenterPoints(); //establish figures for this rental result += "\t" + each.tape().movie().identify()+ "\t" + String.valueOf(each.charge()) + "\n"; } //sum up pedestrian lines consequence += "Add up owed is " + String.valueOf(charge()) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " common renter points"; return result; } snobbish double charge(){ double result = 0; Enumeration rentals = _rentals.elements(); while (rentals.hasMoreElements()) { Rental each = (Property) rentals.nextElement(); result += each.charge(); } return result; } Later on compiling and testing that refactoring, I then did the unchanged for frequentRenterPoints .
public String out statement() { Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + figure() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Material possession) rentals.nextElement(); //picture figures for each rental result += "\t" + each.tape().movie().name()+ "\t" + Cosmic string.valueOf(each.burster()) + "\n"; } //attention deficit hyperactivity disorder footer lines event += "Amount payable is " + Thread.valueOf(charge()) + "\n"; answer += "You earned " + String.valueOf(frequentRenterPoints()) + " shop renter points"; return result; } private int frequentRenterPoints() { int answer = 0; Enumeration rentals = _rentals.elements(); while (rentals.hasMoreElements()) { Rental each = (Annuity in advance) rentals.nextElement(); result += apiece.frequentRenterPoints(); } return result; } It is worth stopping and thinking a bit about this refactoring. Most refactoring reduce the amount of code, but this single increases IT. That's because Java requires a lot of statements to determined up a summing loop. Even a simple summing loop with uncomparable line of code per element needs six lines of support around it. Information technology�s an idiom that is obvious to any software engineer but it is haphazardness that hides what the intent of the loop is. As Java develops and builds up its ability to handgrip block closures in the style of Smalltalk, I expect that overhead to fall, plausibly to the single line that such an reflection would involve in Smalltalk.
The other concern with this refactoring lies in performance. The old code executed the while loop once, the new inscribe executes it cardinal times. If the spell loop takes time, this might significantly spoil performance. Many programmers would not practise this refactoring merely for this reason. But note the dustup "if" and "might". While some loops do cause performance issues, most do not. So while refactoring don�t headache about this. When you optimise you bequeath hold to worry about it, but you volition then cost in a much better position to do something about it, and you will have more options to optimise effectively. (For a peachy discussion happening why information technology is better to write intelligibly first so optimize, see [McConnell, Code Complete].
These queries are now available to whatsoever code written in the customer socio-economic class. Indeed they fanny easily be added to the port of the class should other parts of the system deman this selective information. Without queries like these, other methods demand to deal with knowing about the rentals and building the loops. In a interlocking system that will lead to much to a greater extent code to write and maintain.
You can fancy the difference immediately with the htmlStatement() . I am now at the point where I start out my refactoring hat and put on my adding serve chapeau. I can write htmlStatement() equal this (and add an appropriate trial run).
public String htmlStatement() { Enumeration rentals = _rentals.elements(); String result = "<H1>Rentals for <Mutton>" + constitute() + "</EM></H1><P>\n"; piece (rentals.hasMoreElements()) { Letting each = (Rental) rentals.nextElement(); //show figures for each rental result += each.magnetic tape().movie().name()+ ": " + String.valueOf(each.commit()) + "<BR>\n"; } //add footer lines result += "<P>You owe <EM>" + String.valueOf(charge()) + "</EM><P>\n"; result += "On this rental you earned <EM>" + String.valueOf(frequentRenterPoints()) + "</EM> frequent renter points<P>"; return result; } There is still some codification copied from the ascii version, but that is primarily overdue to setting up the loop. Boost refactoring could clean that up further, extracting methods for heading, footer, and detail line are one route I could take. Just that isn�t where I want to spend my time, I would similar to move onto the methods I�ve moved onto annuity in advance. Back happening with the refactoring hat.
Moving the Rental Calculations to Movie
Yes it�s that switch statement that is bugging me. Information technology is a bad idea to practice a switch supported on an attribute of some other object. If you must use a switch statement, it should get on your own data, not on someone else�s. This implies that the charge should move onto movie
Class movie � double armorial bearing(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) solution += (daysRented - 3) * 1.5; break; } return result; } For this to work I deliver to pass in the length of the rental, which of course is data expected 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 opt to flip the length of rental preferably than the picture�s type? Its because type information tends to be to a greater extent volatile. I can well reckon untried types of videos appearing. If I modify the movie�s type I want the least ripple effect, so I choose to figure out the charge within the movie.
I compiled the method into motion-picture show and then adjusted the charge method acting happening rental to economic consumption the new method acting.
Class rental� double charge() { return _tape.movie().charge(_daysRented); } Some people would prefer to remove that chain of calls by having a armorial bearing(int) message on tape. This would lead to
Class belongings double charge() { take _tape.charge(_daysRented); } Class mag tape double charge() { return _movie.charge up(_daysRented); } You can make that change if you like, I don�t be given to worry about message irons providing that they all lie in the same packet. If they cross package boundaries, so I�m non and so happy, and would add an insulating method.
Having done this with charge amounts, I�m inclined to do the Sami with frequent renter points. The need is less pressing, just I conceive it is Sir Thomas More consistent to coiffe them both the same way. And once more if the movie classifications change it makes it easier to update the code.
Class rental� int frequentRenterPoints() { return _tape.movie().frequentRenterPoints(_daysRented); } course of study movie� int frequentRenterPoints(int daysRented){ if ((priceCode() == NEW_RELEASE) && daysRented > 1) take 2; else return 1; } With these two changes I can hide those constants, which is generally a Good Thing. Flatbottom constant data should be private.
private electrostatic final exam int CHILDRENS = 2; private static final int REGULAR = 0; snobbish static final int NEW_RELEASE = 1;
To really manage this, notwithstandin, I need to modify a few other parts of the class. I need to change how we create a movie. I used to create a movie with a content like-minded
new Movie ("Ran", Movie.REGULAR); and the constructor
class Movie� privy Movie(String name, int priceCode) { _name = name; _priceCode = priceCode; } To keep this typecast code hidden I need some creation methods.
public static Movie newNewRelease(String mention){ return new Movie (public figure, NEW_RELEASE); } public atmospheric static Picture show newRegular(String name){ return red-hot Flic (name, REGULAR); } public static Motion-picture show newChildrens(String name) { generate new Moving picture (name, CHILDRENS); } Now I create a new picture with
Movie.newRegular("Monty Python and the Sangraal"); Movies can change their compartmentalization. I change a movie�s classification with
aMovie.setPriceCode(Movie.REGULAR);
I will need to add a lot of methods to handle the changes of classification.
overt nullity 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 sort of moving-picture show you are getting. This makes the code more comprehensible. The trade off is that each time I add a terms code I cause to add a creation and update method. If I had lots of price codes this would hurt (thus I wouldn�t do it). If I have a some, however, then information technology�s quite reasonable.
At last� inheritance
Thus we have several types of flic, which ingest different slipway of answering the same question. This sounds like a job for subclasses. We could have three subclasses of movie, each of which can take up its own reading of charge.
This would allow me to replace the switch statement by victimization polymorphism. Woefully IT has one tenuous fault: it doesn�t work. A move butt deepen 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.
Past adding the indirection we can dress the subclassing from the price code object, changing the price whenever we need to.
With a colonial class you have to move data and methods approximately in small pieces to avoid errors, it seems slow but information technology is the fastest because you avoid debugging. For this instance I could probably movement the information and methods in one move back as the whole thing is not too complex. However I�ll do it the piecemeal way of life, so you can see how it goes. Fair-minded retrieve to have it off one small flake at once if you do this to a complicated class.
The first step is to create the newly classes. Then I need to form out how they are managed. Atomic number 3 the diagram shows they are all singletons. Information technology seems sensible to get hold of them via the superclass with a method like Mary Leontyne Pric.regular() . I canful do this by acquiring the superclass to manage the instances of the subclasses.
abstract class Damage { static Terms weak() { return _regular; } electricity Price childrens() { return _childrens; } static Price newRelease() { return _newRelease; } private static Price _childrens = new ChildrensPrice(); private static Cost _newRelease = new NewReleasePrice(); private static Price _regular = new-sprung RegularPrice(); } Instantly I can begin to move the data over. The eldest piece of information to move terminated is the price inscribe. Of flow from I�m non in reality exit to use the price encode within the Price targe, but I volition pay IT the semblance of doing so. That way the old methods will still work. They key is to change those methods that access and update the monetary value cipher rate within Movie. My initiative is to self-capsulise the type code , ensuring that totally uses of the type code go though getting and setting methods. Since most of the code came from another classes, most methods already role the getting method. However the constructors do access the terms codification, I can use the setting methods instead.
public static Motion picture newNewRelease(String name){ Movie result = unaccustomed Movie (name); result.beNewRelease(); return ensue; } public static Movie newRegular(String name){ Movie result = new Motion-picture show (name); result.beRegular(); return result; } public static Movie newChildrens(String name) { Film result = novel Movie (name); final result.beChildrens(); retort result; } private Movie(String name) { _name = name; } Later on compiling and examination I now change getting and setting methods to use the inexperient category.
public void beRegular() { _price = Price.regular(); } state-supported void beNewRelease() { _price = Price.newRelease(); } public void beChildrens() { _price = Price.childrens(); } public int priceCode() { return _price.priceCode(); } And furnish the priceCode methods connected Damage 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-toffee-nosed again. This is close, I don�t head them having a little fame ahead they bite the rubble.
I can in real time compile and try out and the more complex methods don�t actualize the human beings has changed.
After moving the data I can now first self-propelled methods. My prime object is the charge() method. It is simple to act up.
Class Motion picture� double consign(int daysRented) { return _price.charge(daysRented); } Class Price� double charge(int daysRented) { double result = 0; switch (priceCode()) { case Movie.Diarrheic: result += 2; if (daysRented > 2) resultant += (daysRented - 2) * 1.5; break; case Movie.NEW_RELEASE: result += daysRented * 3; break-dance; case Movie.CHILDRENS: result += 1.5; if (daysRented > 3) termination += (daysRented - 3) * 1.5; split; } return result; } Erstwhile it is moved I can start replacing the vitrine statement with inheritance . I do this aside pickings 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 resolution = 2; if (daysRented > 2) result += (daysRented - 2) * 1.5; return answer; } This will override the parent caseful statement, which I merely leave as it is. I compile and test for this eccentric, then take the next leg, compile and test�. (To make sure I�m executing the subclass code, I like to throw in a deliberate microbe and run it to ensure the tests blow up. Not that I�m paranoid or anything.)
Division 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� conceptual twofold load(int daysRented);
I can now do the same procedure with frequentRenterPoints() . First I move the method finished to Price .
Class Motion-picture show� int frequentRenterPoints(int daysRented){ return _price.frequentRenterPoints(daysRented); } Class Damage� int frequentRenterPoints(int daysRented){ if ((priceCode() == Film.NEW_RELEASE) &ere;& daysRented > 1) return 2; else return 1; } In this case, however I won�t make the superclass method abstract. Instead I will create an predominant method acting for new releases, and leave a defined method (as the nonremittal) connected the superclass.
Course of instruction NewReleasePrice int frequentRenterPoints(int daysRented){ return (daysRented > 1) ? 2: 1; } Class Price� int frequentRenterPoints(int daysRented){ return 1; } Now I bear removed all the methods that needed a price encode. Sol I can abolish the price code methods and data on both Movie and Price .
Putting in the state pattern was rather an effort, was it valuable it? The earn is now that should I change any of cost�s behavior, add new prices, or add extra price dependent behavior; it will glucinium much easier to change. The stay of the application does non get laid about the use of the state pattern. For the petite amount of behavior I currently have it is not a loud deal. But in a more than complex organisation 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, simply not once did I have to open the debugger. So the process actually flowed quite a quick.
Final Thoughts
This is a simple example, yet I desire it gives you the feeling of what refactoring is like. I�ve used several refactoring techniques: oncoming behavior, replacement case statements, method descent. All these lead to a better distributed responsibilities, and code that is easier to maintain. It does look preferably different to procedural style code, and that does take whatever getting victimized to. But once you are used to it, it is hard to go support to procedural programs.
The nigh important lesson from this object lesson is the rhythm of refactoring: test, small change, test, small change, test, small alter. It is that cycle that allows refactoring to move quickly and safely.
References
[Fowler]
Fowler M with Robert Falcon Scott K, UML Distilled: Applying the Standard Object Modeling Language, Addison-Wesley, Reading MA, 1997
[Gang of Four]
Vasco da Gamma E, Helm R, LBJ R, and Vlissides J, Design Patterns: Elements of Reusable Object Familiarized Software, Addison-Charles Wesley, Reading MA, 1995
[McConnell, Code Complete]
McConnell Steve, Code Pure: A Practical Handbook of Software Construction, Microsoft Press, 1993
Getting Tickets to Fans With Listing Refactor Question
Source: https://www.cs.unc.edu/~stotts/COMP204/refactor/chap1.html
Post a Comment