There are two broad categories of refactorings: the architectural or redesign refactorings, and the "little" refactorings.
The architectural refactorings readjust the interaction of various parts of the code base. They are used to clean the code at a higher level. I am not just cleaning up the code, I'm redesigning on the fly.
For architectural refactorings, the emphasis is on class extraction and fixing the interaction between classes. When I'm done the interface between classes and groups of classes is clean and simple. Individual classes have a single conceptualization and are generally small and simple.
The little refactorings are meant to clean the code at a low level. When I'm done the code for individual classes is clean, simple and clear. The code is transparent in it what it is trying to achieve. I can read the code and with little effort or comments determine what it is doing.
When both types of refactorings are complete, the code is easy to follow at a high level, the architecture is transparent and how each component of the architecture achieves its function is transparent. The code base is more maintainable when I'm done because other developers can read the code quickly and easily.
Let me back up a little. The code is bad right? Otherwise I wouldn't be in there except for pedagogic reasons. If I were refactoring strictly for pedagogy, I'm betting that I'll learn more refactoring bad code then polishing good code.
There are intertwined classes in the code. If a class has a single, simple purpose then almost by default the code will be clear and simple. The only code in the class is directly related to it's purpose and it's purpose is supported by all the code in the class.
When there are two entangled classes in a piece of code, they interfere with each other, creating a sort of moire pattern in my understanding of what the code is trying to do. If the interference is weak, I can still make out what is going on but if the interference is strong, it may overwhelm my attempts at understanding it.
The fewer the number of methods the better. The fewer the number of parameters on a method the better. Most or all of the methods should be involved when typically using the class (if there is a partition of methods, then the class should be split).
The interaction amongst the public methods also affects the simplicity of the
interface. A common example is the Open() method must be called before the Close()
method and the Close() must be called before the Open() is called again. There is
a dependency between Open() and Close() that is not apparent by just looking at
the signatures of the methods. The dependency, to be made clear to the user of the
class, must be documented somewhere or be so idiomatic (as it is in this case) that
the use already knows it
I like this rule very much because it reinforces what "merciless" really means. When I feel like I've done enough refactoring on a class, I double check the number of lines in each method. If there are any over 1, I justify them to myself or continue refactoring them.
The rule motivates me to extract code and methods. The end result are final classes that are very simple and singular in purpose.
If I have a perfectly clean code base, say the very first line of code of a brand new application I'm writing. I refactor it mercilessly. I then add two or three lines of new code and refactor mercilessly. Compare that to refactoring a method with 250 lines of code.
The mental or psychological effort is nearly trivial in the first case. Any "code smells" are readily apparent. Complicated areas of code are easily found and corrected. The effort is substantially more for the legacy code. The qualitative difference is perhaps due to mental fatigue or some sort of psychological phenomenon, but it is real and translates into more time spent performing the refactoring.
XP's test-first, UT and FT (functional test) practices work very well in the forward direction: I start with a clean system, add clean pieces and end with a clean system. But I am going the other way. I never know and will never know that I have a clean system in the way that XP allows me to know.
I do not do global replaces. If there is a code change in 10 places, I do the first in its entirety before I move onto the next place. I recompile and run the ATs at least once per each of the 10 places.
Sometimes I've been forced into doing a large change. I prepare for these by writing a quick backup script, e.g. copy all the source files to a backup directory. I make sure the backup script works and try rolling back to the backup versions. Then I proceed with the large change.
I've found a source control tool has been generally too clumsy for this kind of work. The simple backup is quicker, easier to rollback and I can have multiple versions of backup. I can also use my favorite full-directory tree comparison tool at will.
The objects put into Globals have to be global for the lifetime of the session and generally used by the rest of the system. If it is not possible to initialize all Global variables, I put an Init() method in Global and call it from the mainline to initialize.
I add methods to Global as needed to access the Global instances. I try to create more than just setter/getter methods. I move as much code surrounding the use of a Global instance into the new Global method as I can.
I don't use Globals and Constants loosely. On the other hand, I don't agonize whether or not to put an object into them. If later on I feel that something should not be global, I just extract it out. The Globals class usually starts out as a dumping ground. I need to have a place somewhere in the system where I can quickly place methods and variables. I don't want to slow down the work just because I can't find a spot to put a particular method.
Eventually, I find that most variables I put in this class are not globals after all. I tend to place as much as code as I can into Globals and then extract a class from it every so often. When the refactoring is done, Globals is usually empty or nearly empty. Anything left over should be truly a global method or instance: if my entire code base uses it, it is a global. If Globals is empty, I delete it.
Finding these 10 lines is sometimes very easy: there is a comment heading them up explaining what they do, or there is a blank line separating groups of lines. In other words, the original programmer intuitively knew that the lines belonged together but didn't write a method to group them. I just help the code along by extracting the method.
The formal parameters of the methods are the instance variables of the new class. The extracted methods then have empty or nearly empty parameter lists.
I create the new class as soon as I can, but I take a moment to think about why those particular methods group together. I name the new class based on that relationship, that becomes the conceptualization of the class.
While I do this, I keep track of how the methods in the new class are fitting together. Do they, as a group, make sense under the class's name? If I need to, I change the name of the class to better reflect what the class is now doing. If the class has gotten too complex, I've gone too far and I need to split the new class.
I look at the code surrounding instance variables of a class. If there is repetitive use with exact or similar code around the instance variable, I extract a method.
If I've extracted several methods that all use the same instance variable, I see if I can extract out a new class.
If the API calls are being applied to the same sort of object, e.g. the code is only opening an ini file, I extract out the new methods into a new class, e.g. an ini file class.
I periodically look at the Global methods with this rule in mind. Can I partition them into logical associations? If so, I group the methods into a new class. I move as much of the Global code as I can into the new object.
Do some of the methods only interact with a few instance variables and those instance variables are only used by those methods? If so, I extract those methods and instance variables into a new class. I move as much code as possible into the new class. See if you can turn the calls to those objects inside out: instead of an object calling a method on the Global class with a set of parameters, see if you can pass the object into the Global method. If it makes sense to do so for a number of Global methods, extract those methods out into a new class. Doing all of the above might cause you to create objects that really don't stand alone, so you may end up merging two objects after the fact. If you find that you have split a class, merged it and then split it again, stop. Find out why you are thrashing. While you're creating the objects, try to develop a deeper understanding of how the application is structured. Creating and naming new objects becomes a whole lot easier if you know what the major components of the code are and how they relate to each other. You might even be able to predict the existence of future objects this way and make it a self-fulfilling prophecy. If you find Globals and/or Constants empty, then delete them. I didn't forget them! At some point in time, you will not be able to discover any new objects using the above methods. Try some other techniques to change your perspective and to take a breather: Review Refactoring by Michael ?? and apply all his techniques to your code. At this point, UTs (unit tests) may be helpful. Balance this off with the effort of writing the UTs. Have someone else look at the code. If they cannot understand the intent of the code easily, refactor the code until it's structure and intent is easily apparent to both of you. Resist adding comments! You can however, leave class level comments to clarify why that class exists and any notes on how to use it. It is better to change the code such that the comments are not necessary, e.g. // jump to the next date curdate += 10; curdate %= get_days_of_the_month(); refactored to: jump_to_the_next_date(currdate); In the second case a comment is not required. Apply a reengineering tool to your new code. What does the UML look like? Does it make sense? Do any of the classes look like "Controller" classes? Are there any classes that are disconnected? Do the class names make sense from this perspective? Check the top level interfaces. Can you change them to reduce the total amount of code in the system? If so, keep the new macro- architecture in mind while you refactor at a micro level. You can slowly convert to the new architecture taking small and safe steps along the way. Also flipping back and forth from the micro- to the macro- level can give you a different perspective. Things discovered at the macro- level can give you a clue to overall program structure and therefore to new objects. Look at the overall class structure of your program. Apply these good OO heuristics: Is the class structure flat? Reduce the hierarchy depth as much as makes sense. Is there associations between classes? Are classes as dumb to the outside as possible? Reduce associations between classes to a minimum by making each class dumb about everything outside itself. Does any class know about other classes in the rest of the program? Refactor the class so that it knows only about itself. There are two exceptions to these rules. First is that each class lives in the operating system or environment provided libraries. I freely include those anywhere they may be useful. The other exception is the main program or a small number of control objects. These act as glue to tie together all of the tools to solve the problem at hand. You may object that this makes it more difficult to port to a new OS or library. I would argue that a well factored application using good OO style is more important to achieve than portability. It is of no use to sacrifice code maintainability and readability in the medium term (a few years) for a long term goal that may or may not come to fruition. I think that cross-platform compatibility is overrated as a requirement although there have been tragedies because it wasn't. Simply said, cross-platform techniques are very difficult to do and to get right. Languages like Java and Perl mitigate these problems but do not make them go away. I would say the vast majority of applications would be better off if they were done right for one platform rather than done badly for several. -- remove dead code -- Use Idempotency -- use a class to encapsulate many formal parameters. --- refactoring list post... *Merciless* Refactoring is a crucial step in the cycle. I'd try this: - back out/comment out the UT that's not passing. Now all UTs pass. - refactor the code until it is as clean as I can make it and then I'd go one step further. If I refactor past the point of reasonableness, chances are I'll hit the merciless zone just barely on the left hand side. - I'd flip-flop between these two refactorings: - Extract method - move temporaries as close as possible to their first-use point - extract the temporary and it's surrounding code into a method - look for duplicate code similar to the method I just extracted and replace it with a method call - look for lines of code that use the same instance variable, or same temporary. Extract them into a method. - Extract class - I'd look for places where I could extract a very simple class even if it had only one method and one member variable. I'd use the class as a lightening :) rod for moving more methods into the class. - I'd break up a big method into a class. The old temporaries and formal parameters to the method become instance variables in the class. All duplication within the old method will disappear as I refactor it's contents into the new class. That done, I'd look at making the new class an instance variable in the original class and getting rid of the old method entirely. - I'd look for series of methods with similar signatures. I'd extract them into a new class. The replication within the signatures is code duplication. The class extraction gets rid of that duplication. - I'd look for series of methods with similar prefixes or suffixes in their names. e.g. BobInit(), BobTerm(), BobOpen(), BobClose() strongly suggest that I make Bob a class. - I'd UT and refactor the new classes mercilessly. - I'm done when: - each class has a singular purpose. A quick *indirect* measure of this: there will be very few, if any, 'if' statements in any one class. (If not, I'd look at moving the 'if's higher up in the calling chain and using whatever design patterns I could to remove the 'if's.) Another quick *indirect* measure of this: there are only 1 or 2 member variables in the class. More are ok but I'd have to justify their existence. - there's no more duplication. A quick *indirect* measure for this: avg # of statements per method => between 1 and 3. - the class structure/architecture is clean and representative of the domain When I'm done, I'd re-add the one test and then it should be substantially easier to get it to pass. It sounds long but 2 or 3 hours at first and then 5 minutes per UT thereafter ought to do it.