Refactoring is the process of modifying code without affecting either its input or output; that is, the code's interface remains the same. How do we know that refactoring works? The only objective way to find the answer is by testing the code before and after refactoring occurs. The results should match exactly. This makes testing an integral part of any refactoring effort--you cannot successfully separate the two.

Some confusion exists between what it means to refactor code as opposed to reworking it. Refactoring is a subset of reworking. If you are making no changes to the interface of a program, you are refactoring. If you add or change the interface, you are rewriting the code. Strictly speaking, when you do both together, you are rewriting. However, for political reasons, you still might want to call it refactoring.

When it comes to methodology, refactoring usually is done in small steps. After each step, you run those unit tests originally written for the existing code. Repeat these two steps as needed. Also, a number of refactoring patterns can be followed. Martin Fowler's book (see Resources) contains some basic ones, and Joshua Kerievsky's more recent text (see Resources) contains some more advanced ones. As a side issue, it is interesting to note that most books and products dealing with refactoring use Java as the language of choice. Is that because Java is the most appropriate language, or does Java code need more refactoring?

All the refactoring tools I have seen so far require heavy human intervention. They don't do pattern recognition, so they aren't able to see commonality in code or design.

As noted above, a lot of refactoring tools are out there, especially for Java. Open-source tools are available, some of which are listed in Resources. Commercial refactoring products can be found in the usual way. In this article, I discuss two open-source tools, Eclipse and Bicycle Repair Man. Eclipse is designed for use with Java, while Bicycle Repair Man is designed to be used with Python.

Let's start with Eclipse and Java. Eclipse offers an impressive set of refactoring facilities, including support for:

To give you some idea of what Eclipse's refactoring can do, I chose at random the Java program shown in Listing 1. It outputs an About dialog box.

Listing 1. Random Java Program

package junit.awtui;

import java.awt.*;
import java.awt.event.*;

import junit.runner.Version;

class AboutDialog extends Dialog {
    public AboutDialog(Frame parent) {
        super(parent);

        setResizable(false);
        setLayout(new GridBagLayout());
        setSize(330, 138);
        setTitle("About");

        Button button= new Button("Close");
        button.addActionListener(
            new ActionListener() {
                public void actionPerformed(ActionEvent e) {
                    dispose();
                }
            }
        );

        Label label1= new Label("JUnit");
        label1.setFont(new Font("dialog", Font.PLAIN, 36));

        Label label2= new Label("JUnit "+
                                Version.id()+ 
                                " by Kent Beck and Erich Gamma");
        label2.setFont(new Font("dialog", Font.PLAIN, 14));

        Logo logo= new Logo();

        GridBagConstraints constraintsLabel1= new GridBagConstraints();
        constraintsLabel1.gridx = 3; constraintsLabel1.gridy = 0;
        constraintsLabel1.gridwidth = 1; constraintsLabel1.gridheight = 1;
        constraintsLabel1.anchor = GridBagConstraints.CENTER;
        add(label1, constraintsLabel1);

        GridBagConstraints constraintsLabel2= new GridBagConstraints();
        constraintsLabel2.gridx = 2; constraintsLabel2.gridy = 1;
        constraintsLabel2.gridwidth = 2; constraintsLabel2.gridheight = 1;
        constraintsLabel2.anchor = GridBagConstraints.CENTER;
        add(label2, constraintsLabel2);

        GridBagConstraints constraintsButton1= new GridBagConstraints();
        constraintsButton1.gridx = 2; constraintsButton1.gridy = 2;
        constraintsButton1.gridwidth = 2; constraintsButton1.gridheight = 1;
        constraintsButton1.anchor = GridBagConstraints.CENTER;
        constraintsButton1.insets= new Insets(8, 0, 8, 0);
        add(button, constraintsButton1);

        GridBagConstraints constraintsLogo1= new GridBagConstraints();
        constraintsLogo1.gridx = 2; constraintsLogo1.gridy = 0;
        constraintsLogo1.gridwidth = 1; constraintsLogo1.gridheight = 1;
        constraintsLogo1.anchor = GridBagConstraints.CENTER;
        add(logo, constraintsLogo1);

        addWindowListener(
            new WindowAdapter() {
                public void windowClosing(WindowEvent e) {
                    dispose();
                }
            }
        );
    }
}

First, I scanned the code and saw that two similar fragments of code dealt with labels. Second, I manually brought together all the code to do with labels. Using similar logic, I brought together the code dealing with the button and the logo. This step is shown in Listing 2.

Listing 2. Rearranging the Code

package junit.awtui;

import java.awt.*;
import java.awt.event.*;

import junit.runner.Version;

class AboutDialog extends Dialog {
    public AboutDialog(Frame parent) {
        super(parent);

        setResizable(false);
        setLayout(new GridBagLayout());
        setSize(330, 138);
        setTitle("About");

        Label label1= new Label("JUnit");
        label1.setFont(new Font("dialog", Font.PLAIN, 36));

        Label label2= new Label("JUnit "+
                    Version.id()+ 
                    " by Kent Beck and Erich Gamma");
        label2.setFont(new Font("dialog", Font.PLAIN, 14));

        Logo logo= new Logo();

        GridBagConstraints constraintsLabel1= new GridBagConstraints();
        constraintsLabel1.gridx = 3; 
        constraintsLabel1.gridy = 0;
        constraintsLabel1.gridwidth = 1; 
        constraintsLabel1.gridheight = 1;
        constraintsLabel1.anchor = GridBagConstraints.CENTER;
        add(label1, constraintsLabel1);

        GridBagConstraints constraintsLabel2= new GridBagConstraints();
        constraintsLabel2.gridx = 2; 
        constraintsLabel2.gridy = 1;
        constraintsLabel2.gridwidth = 2; 
        constraintsLabel2.gridheight = 1;
        constraintsLabel2.anchor = GridBagConstraints.CENTER;
        add(label2, constraintsLabel2);

        Button button= new Button("Close");
        button.addActionListener(
            new ActionListener() {
                public void actionPerformed(ActionEvent e) {
                    dispose();
                }
            }
        );
        GridBagConstraints constraintsButton1=new GridBagConstraints();
        constraintsButton1.gridx = 2; 
        constraintsButton1.gridy = 2;
        constraintsButton1.gridwidth = 2; 
        constraintsButton1.gridheight = 1;
        constraintsButton1.anchor = GridBagConstraints.CENTER;
        constraintsButton1.insets= new Insets(8, 0, 8, 0);
        add(button, constraintsButton1);

        GridBagConstraints constraintsLogo1= new GridBagConstraints();
        constraintsLogo1.gridx = 2; 
        constraintsLogo1.gridy = 0;
        constraintsLogo1.gridwidth = 1; 
        constraintsLogo1.gridheight = 1;
        constraintsLogo1.anchor = GridBagConstraints.CENTER;
        add(logo, constraintsLogo1);

        addWindowListener(
            new WindowAdapter() {
                public void windowClosing(WindowEvent e) {
                    dispose();
                }
            }
        );
    }
}

Next, I extracted each method by first highlighting each method and then selecting Refactor | Extract Method. The dialog box is shown below.

The resulting code is shown in Listing 3.

Listing 3. Refactored Code

package junit.awtui;

import java.awt.*;
import java.awt.event.*;

import junit.runner.Version;

class AboutDialog extends Dialog {
    public AboutDialog(Frame parent) {
        super(parent);

        setResizable(false);
        setLayout(new GridBagLayout());
        setSize(330, 138);
        setTitle("About");


        gbLabel1();
        gbLabel2();
        gbButton();
        gbLogo();

        addWindowListener(
            new WindowAdapter() {
                public void windowClosing(WindowEvent e) {
                    dispose();
                }
            }
        );
    }

        /**
         *
         */
        private void gbLogo() {
            Logo logo= new Logo();
            GridBagConstraints constraintsLogo1= new GridBagConstraints();
            constraintsLogo1.gridx = 2; 
            constraintsLogo1.gridy = 0;
            constraintsLogo1.gridwidth = 1; 
            constraintsLogo1.gridheight = 1;
            constraintsLogo1.anchor = GridBagConstraints.CENTER;
            add(logo, constraintsLogo1);
        }

        /**
         *
         */
        private void gbButton() {
            Button button= new Button("Close");
            button.addActionListener(
                new ActionListener() {
                    public void actionPerformed(ActionEvent e) {
                        dispose();
                    }
                }
            );
            GridBagConstraints constraintsButton1= new GridBagConstraints();
            constraintsButton1.gridx = 2; 
            constraintsButton1.gridy = 2;
            constraintsButton1.gridwidth = 2; 
            constraintsButton1.gridheight = 1;
            constraintsButton1.anchor = GridBagConstraints.CENTER;
            constraintsButton1.insets= new Insets(8, 0, 8, 0);
            add(button, constraintsButton1);
        }

        /**
         *
         */
        private void gbLabel2() {
            Label label2= new Label("JUnit "+
                                    Version.id()+ 
                                    " by Kent Beck and Erich Gamma");
            label2.setFont(new Font("dialog", Font.PLAIN, 14));
            GridBagConstraints constraintsLabel2= new GridBagConstraints();
            constraintsLabel2.gridx = 2; 
            constraintsLabel2.gridy = 1;
            constraintsLabel2.gridwidth = 2; 
            constraintsLabel2.gridheight = 1;
            constraintsLabel2.anchor = GridBagConstraints.CENTER;
            add(label2, constraintsLabel2);
        }

        /**
         *
         */
        private void gbLabel1() {
            Label label1= new Label("JUnit");
            label1.setFont(new Font("dialog", Font.PLAIN, 36));
            GridBagConstraints constraintsLabel1= new GridBagConstraints();
            constraintsLabel1.gridx = 3; 
            constraintsLabel1.gridy = 0;
            constraintsLabel1.gridwidth = 1; 
            constraintsLabel1.gridheight = 1;
            constraintsLabel1.anchor = GridBagConstraints.CENTER;
            add(label1, constraintsLabel1);
        }
}

Looking at the code again, it seems that gbLabel1 and gbLabel2 share the same logic, but the values of some of the variables differ slightly. Manually comparing the two, I tried to create a new method based on gbLabel1 that had parameters on those variable values that differed between gbLabel1 and gbLabel2. I did this by using the Introduce Parameter refactoring wizard. Listing 4 shows the result of using this wizard to replace the label's text and font values with method parameters.

Listing 4. Replacing Values with Parameters

package junit.awtui;

import java.awt.*;
import java.awt.event.*;

import junit.runner.Version;

class AboutDialog extends Dialog {
    public AboutDialog(Frame parent) {
        super(parent);

        setResizable(false);
        setLayout(new GridBagLayout());
        setSize(330, 138);
        setTitle("About");

        gbLabel1(new Label("JUnit"),new Font("dialog",Font.PLAIN, 36));
        gbLabel2();
        gbButton();
        gbLogo();

        addWindowListener(
            new WindowAdapter() {
                public void windowClosing(WindowEvent e) {
                    dispose();
                }
            }
        );
    }

        /**
         *
         */
        private void gbLogo() {
            Logo logo= new Logo();
            GridBagConstraints constraintsLogo1= new GridBagConstraints();
            constraintsLogo1.gridx = 2; 
            constraintsLogo1.gridy = 0;
            constraintsLogo1.gridwidth = 1; 
            constraintsLogo1.gridheight = 1;
            constraintsLogo1.anchor = GridBagConstraints.CENTER;
            add(logo, constraintsLogo1);
        }

        /**
         *
         */
        private void gbButton() {
            Button button= new Button("Close");
            button.addActionListener(
                new ActionListener() {
                    public void actionPerformed(ActionEvent e) {
                        dispose();
                    }
                }
            );
            GridBagConstraints constraintsButton1= new GridBagConstraints();
            constraintsButton1.gridx = 2; 
            constraintsButton1.gridy = 2;
            constraintsButton1.gridwidth = 2; 
            constraintsButton1.gridheight = 1;
            constraintsButton1.anchor = GridBagConstraints.CENTER;
            constraintsButton1.insets= new Insets(8, 0, 8, 0);
            add(button, constraintsButton1);
        }

        /**
         *
         */
        private void gbLabel2() {
            Label label2= new Label("JUnit "+
                                    Version.id()+ 
                                    " by Kent Beck and Erich Gamma");
            label2.setFont(new Font("dialog", Font.PLAIN, 14));
            GridBagConstraints constraintsLabel2= new GridBagConstraints();
            constraintsLabel2.gridx = 2; 
            constraintsLabel2.gridy = 1;
            constraintsLabel2.gridwidth = 2; 
            constraintsLabel2.gridheight = 1;
            constraintsLabel2.anchor = GridBagConstraints.CENTER;
            add(label2, constraintsLabel2);
        }

        /**
         * @param lblText TODO
         * @param lblFont TODO
         *
         */
        private void gbLabel1(Label lblText, Font lblFont) {
            Label label1= lblText;
            label1.setFont(lblFont);
            GridBagConstraints constraintsLabel1= new GridBagConstraints();
            constraintsLabel1.gridx = 3; 
            constraintsLabel1.gridy = 0;
            constraintsLabel1.gridwidth = 1; 
            constraintsLabel1.gridheight = 1;
            constraintsLabel1.anchor = GridBagConstraints.CENTER;
            add(label1, constraintsLabel1);
        }
}

Two things in Listing 4 are worth noting. First, in the comments before the newly modified method, TODOs remind you to comment on the meaning of the parameters. Second, in both gbLabel1 and gbLabel2 the placement of the label is specified using a rectangle with its upper-left corner and height and width values. Eclipse cannot recognize these as attributes of a rectangle; it must be done manually. I used the Change Method Signature wizard to introduce the Rectangle parameter.

Besides changing the method signature, I also needed to change manually the code where gbLabel1 is invoked. Listing 5 shows the results with some minor formatting changes being added.

Listing 5. Changing gbLabel1 Invocation Code

package junit.awtui;

import java.awt.*;
import java.awt.event.*;

import junit.runner.Version;

class AboutDialog extends Dialog {
    public AboutDialog(Frame parent) {
        super(parent);

        setResizable(false);
        setLayout(new GridBagLayout());
        setSize(330, 138);
        setTitle("About");


        gbLabel1(new Label("JUnit"),
             new Font("dialog", Font.PLAIN, 36),
             new Rectangle(3,0,1,1));
        gbLabel2();
        gbButton();
        gbLogo();

        addWindowListener(
            new WindowAdapter() {
                public void windowClosing(WindowEvent e) {
                    dispose();
                }
            }
        );
    }

        /**
         *
         */
        private void gbLogo() {
            Logo logo= new Logo();
            GridBagConstraints constraintsLogo1= new GridBagConstraints();
            constraintsLogo1.gridx = 2; 
            constraintsLogo1.gridy = 0;
            constraintsLogo1.gridwidth = 1; 
            constraintsLogo1.gridheight = 1;
            constraintsLogo1.anchor = GridBagConstraints.CENTER;
            add(logo, constraintsLogo1);
        }

        /**
         *
         */
        private void gbButton() {
            Button button= new Button("Close");
            button.addActionListener(
                new ActionListener() {
                    public void actionPerformed(ActionEvent e) {
                        dispose();
                    }
                }
            );
            GridBagConstraints constraintsButton1= new GridBagConstraints();
            constraintsButton1.gridx = 2; 
            constraintsButton1.gridy = 2;
            constraintsButton1.gridwidth = 2; 
            constraintsButton1.gridheight = 1;
            constraintsButton1.anchor = GridBagConstraints.CENTER;
            constraintsButton1.insets= new Insets(8, 0, 8, 0);
            add(button, constraintsButton1);
        }

        /**
         *
         */
        private void gbLabel2() {
            Label label2= new Label("JUnit "+
                                    Version.id()+ 
                                    " by Kent Beck and Erich Gamma");
            label2.setFont(new Font("dialog", Font.PLAIN, 14));
            GridBagConstraints constraintsLabel2= new GridBagConstraints();
            constraintsLabel2.gridx = 2; 
            constraintsLabel2.gridy = 1;
            constraintsLabel2.gridwidth = 2; 
            constraintsLabel2.gridheight = 1;
            constraintsLabel2.anchor = GridBagConstraints.CENTER;
            add(label2, constraintsLabel2);
        }

        /**
         * @param lblText Text to appear in label
         * @param lblFont Font to use in showing text
         * @param lblRect the size and position of the label
         *
         */
        private void gbLabel1(Label lblText, Font lblFont, Rectangle lblRect) {
            Label label1= lblText;
            label1.setFont(lblFont);
            GridBagConstraints constraintsLabel1= new GridBagConstraints();
            constraintsLabel1.gridx = lblRect.x;
            constraintsLabel1.gridy = lblRect.y;
            constraintsLabel1.gridwidth = lblRect.width;
            constraintsLabel1.gridheight = lblRect.height;
            constraintsLabel1.anchor = GridBagConstraints.CENTER;
            add(label1, constraintsLabel1);
        }
}

Next, I noticed that the variable label1 could be replaced by label, because only one variable of this type is in this method. To do this, I used the Rename... wizard. I highlighted the name to be changed and invoked the wizard, as shown below.

Because the gbLabel1 method now has become quite generic, we also can change its name simply to gbLabel. We start by highlighting an instance of its name and invoking the Rename... wizard. Next, we replace the call to gbLabel2 with an equivalent call to gbLabel. Again, this must be done manually. Also, because gbLabel2 is no longer needed, we should delete it. Listing 6 shows the result of all this.

Listing 6. Cleaning Up the Code

package junit.awtui;

import java.awt.*;
import java.awt.event.*;

import junit.runner.Version;

class AboutDialog extends Dialog {
    public AboutDialog(Frame parent) {
        super(parent);

        setResizable(false);
        setLayout(new GridBagLayout());
        setSize(330, 138);
        setTitle("About");

        gbLabel(new Label("JUnit"),
            new Font("dialog", Font.PLAIN, 36),
            new Rectangle(3,0,1,1));
        gbLabel(new Label("JUnit "+
                          Version.id()+ 
                          " by Kent Beck and Erich Gamma"),
            new Font("dialog", Font.PLAIN, 14),
            new Rectangle(2,1,2,1));
        gbButton();
        gbLogo();

        addWindowListener(
            new WindowAdapter() {
                public void windowClosing(WindowEvent e) {
                    dispose();
                }
            }
        );
    }

        /**
         *
         */
        private void gbLogo() {
            Logo logo= new Logo();
            GridBagConstraints constraintsLogo1= new GridBagConstraints();
            constraintsLogo1.gridx = 2; 
            constraintsLogo1.gridy = 0;
            constraintsLogo1.gridwidth = 1; 
            constraintsLogo1.gridheight = 1;
            constraintsLogo1.anchor = GridBagConstraints.CENTER;
            add(logo, constraintsLogo1);
        }

        /**
         *
         */
        private void gbButton() {
            Button button= new Button("Close");
            button.addActionListener(
                new ActionListener() {
                    public void actionPerformed(ActionEvent e) {
                        dispose();
                    }
                }
            );
            GridBagConstraints constraintsButton1= new GridBagConstraints();
            constraintsButton1.gridx = 2; 
            constraintsButton1.gridy = 2;
            constraintsButton1.gridwidth = 2; 
            constraintsButton1.gridheight = 1;
            constraintsButton1.anchor = GridBagConstraints.CENTER;
            constraintsButton1.insets= new Insets(8, 0, 8, 0);
            add(button, constraintsButton1);
        }

        /**
         * @param lblText Text to appear in label
         * @param lblFont Font to use in showing text
         * @param lblRect the size and position of the label
         *
         */
        private void gbLabel(Label lblText, Font lblFont, Rectangle lblRect) {
            Label lbl= lblText;
            lbl.setFont(lblFont);
            GridBagConstraints constraintsLabel1= new GridBagConstraints();
            constraintsLabel1.gridx = lblRect.x;
            constraintsLabel1.gridy = lblRect.y;
            constraintsLabel1.gridwidth = lblRect.width;
            constraintsLabel1.gridheight = lblRect.height;
            constraintsLabel1.anchor = GridBagConstraints.CENTER;
            add(lbl, constraintsLabel1);
        }
}

If you wish, you can continue on the same track with both the button and logo related methods. However, it is more of the same, so I won't bore you further on using refactoring with Eclipse and Java.

The standard refactoring tool for Python is Bicycle Repair Man (BRM). Its feature set is more limited and less mature than Eclipse's set, in part because of the language differences and in part because of the number of developers working on them. Bicycle Repair Man's current refactoring wizards are Rename, Extract method, Inline variable, Extract variable, Move class and Move function.

For this test, I chose a program I knew something about--the token handler for PyMetrics. Last month I wrote about code metrics and PyMetrics. I did not like how I coded the PyMetrics routine shown below, and its McCabe metric of 52 for the __call__ method gave me reason to refactor and rework the code. Recall that the McCabe Cyclomatic metric measures the number of paths through your function or procedure. At 52, this method is considered extremely high risk and untestable. Listing 7 shows the __call__ method that I wanted refactored.

Listing 7. PyMetric __call__ Method

""" TokenHandler readies tokens to passing to metric routines.

  $Id: tokenhandler.py,v 1.2 2005/01/19 06:13:33 rcharney Exp $
"""
__version__ = "$Revision: 1.2 $"[11:-2]

#import token
from globals import *

class TokenHandler( object ):
  """ Class to handle processing of tokens."""
  def __call__( self, tokCount, tok ):
    """ Common code for handling tokens."""
    tokCount += 1
    self.metrics['tokenCount'] = tokCount
    if not tok.type in [WS,INDENT,DEDENT,EMPTY,ENDMARKER]:
      prevTok = self.saveTok
      self.saveTok = tok
    self.context['blockNum'] = self.metrics.get('blockCount',0)
    self.context['blockDepth'] = self.metrics.get('blockDepth',0)
    self.context['parenDepth'] = self.parenDepth
    self.context['bracketDepth'] = self.bracketDepth
    self.context['braceDepth'] = self.braceDepth

    self.classDepth += self.classDepthIncr
    self.context['classDepth'] = self.classDepth
    self.classDepthIncr = 0
    
    self.fcnDepth += self.fcnDepthIncr    # incr at start of fcn body
    self.context['fcnDepth'] = self.fcnDepth
    self.fcnDepthIncr = 0
    
    # start testing for types that change in context
    
    if self.docString and tok.type == token.STRING:
      self.docString = False
      tok.semtype = DOCSTRING
      if self.checkForModuleDocString:  # found the module's doc string
        self.metrics['numModuleDocStrings'] += 1
        self.checkForModuleDocString = False
      self.__postToken( tok )
      return
    if tok.type == COMMENT:
      # compensate for older tokenize including newline
      # obals......in token when only thing on line is comment
      # this patch makes all connents consistent
      self.metrics['numComments'] += 1
      if tok.text[-1] == '\n':
        tok.text = tok.text[:-1]
      if prevTok and prevTok.type != NEWLINE and prevTok.type != EMPTY:
        tok.semtype = INLINE
        self.metrics['numCommentsInline'] += 1
      self.__postToken( tok )
      return
      
    if self.defFunction:
      if tok.type == NAME:
        self.__incr( 'numFunctions')
        self.fqnName.append( tok.text )
        self.currentFcn = self.__genFQN( self.fqnName )
        self.fqnFunction.append( (self.currentFcn,self.blockDepth,FCNNAME) )
        self.defFunction = False
        self.findFcnHdrEnd = True
        if self.pa.verbose > 0: print "fqnFunction=%s" % self.fqnFunction
        self.__postToken( tok )
        return
      elif tok.type == ERRORTOKEN:
        # we must compensate for the simple scanner mishandling errors
        self.findFcnHdrEnd = True
        self.invalidToken = mytoken.MyToken(type=NAME, 
                                            semtype=FCNNAME, 
                                            text=tok.text, 
                                            row=tok.row, 
                                            col=tok.col, 
                                            line=tok.line)
        self.skipUntil = '(:\n'
        return
    elif self.defClass and tok.type == NAME:
      self.__incr( 'numClasses' )
      self.fqnName.append( tok.text )
      self.currentClass = self.__genFQN( self.fqnName )
      self.fqnClass.append( (self.currentClass,self.blockDepth,CLASSNAME) )
      self.defClass = False
      self.findClassHdrEnd = True
      if self.pa.verbose > 0: print "fqnClass=%s" % self.fqnClass
      self.__postToken( tok )
      return
      
    # return with types that don't change in context
    
    self.__postToken( tok )
    if tok.type == EMPTY:
      self.endOfStmt()
      return
    if tok.type == WS:
      return
    if tok.type == NEWLINE:
      self.endOfStmt()
      return
    
    # at this point, we have encountered a non-white space token
    # if a module doc string has not been found yet, it never will be.
    numModDocStrings = self.metrics['numModuleDocStrings']
    if self.checkForModuleDocString and numModDocStrings == 0:
      self.checkForModuleDocString = False
      msg = ( "Module %s is missing a module doc string. Detected at line %d"
               % (self.context['inFile'],tok.row) )
      if not self.pa.quietSw:
        sys.stderr.writelines( msg )

    if tok.type == OP and tok.text == ':':
      if self.findFcnHdrEnd:
        self.findFcnHdrEnd = False
        self.docString = True
        self.fcnDepthIncr = 1
        return
      elif self.findClassHdrEnd:
        self.findClassHdrEnd = False
        self.docString = True
        self.classDepthIncr = 1
        return
    if tok.type == OP:
      if tok.text == '(':
        self.parenDepth += 1
      elif tok.text == ')':
        self.parenDepth -= 1
      elif tok.text == '[':
        self.bracketDepth += 1
      elif tok.text == ']':
        self.bracketDepth -= 1
      elif tok.text == '{':
        self.braceDepth += 1
      elif tok.text == '}':
        self.braceDepth -= 1
      return
        
    if tok.type == INDENT:
      self.__incr( 'blockCount')
      self.blockDepth += 1
      self.metrics['blockDepth'] = self.blockDepth
      if self.metrics.get('maxBlockDepth',0) < self.blockDepth:
        self.metrics['maxBlockDepth'] = self.blockDepth
      return
    elif tok.type == DEDENT:
      assert self.fcnDepth == len( self.fqnFunction )
      self.blockDepth -= 1
      self.metrics['blockDepth'] = self.blockDepth
      while len(self.fqnFunction) and self.blockDepth<=self.fqnFunction[-1][1]:
        msg = self.endOfFunction( self.fcnExits )
        if msg and not self.pa.quietSw:
          print msg
        if self.pa.verbose > 0: 
          print "Removing function %s" % self.__extractFQN( self.fqnFunction )
        del self.fqnFunction[-1]
        del self.fqnName[-1]
        self.fcnDepth -= 1
      assert self.fcnDepth == len( self.fqnFunction )
      if len( self.fqnFunction ) > 0:
        self.currentFcn = self.fqnFunction[-1][0]
      else:
        self.currentFcn = None
      while len(self.fqnClass) and self.blockDepth <= self.fqnClass[-1][1]:
        self.endOfClass()
        self.classDepth -= 1
        if self.pa.verbose > 0: 
          print "Removing class %s" % self.__extractFQN( self.fqnClass )
        del self.fqnClass[-1]
        del self.fqnName[-1]
      if len( self.fqnClass ) > 0:
        self.currentClass = self.fqnClass[-1][0]
      else:
        self.currentClass = None
      return

    self.docString = False
    if tok.semtype == KEYWORD:
      self.__incr( 'numKeywords')
      if tok.text == 'def':
        self.defFunction = True
      elif tok.text == 'class':
        self.defClass = True
      elif tok.text == 'return':
        assert self.fcnDepth == len( self.fqnFunction )
        if self.fcnDepth == 0:     # then we are not in any function
          if not self.pa.quietSw:  # then report on these types of errors
            msg = ("Module %s contains the return statement" +
                   " at line %d that is outside any function" % 
                   (self.context['inFile'],tok.row) )
            print msg
        if prevTok.text == ':': 
          # this return on same line as conditional, 
          # so it must be an extra return
          self.fcnExits.append( tok.row )
        elif self.blockDepth > 1:
          # Let fcnBlockDepth be the block depth of the function body.
          # We are trying to count the number of return statements
          # in this function. Only one is allowed at the fcnBlockDepth for the
          # function. If the self.blockDepth is greater than fcnBlockDepth, 
          # then this is a conditional return - i.e., an additional return
          fcnBlockDepth = self.fqnFunction[-1][1] + 1
          if self.blockDepth > fcnBlockDepth:
            self.fcnExits.append( tok.row )
            
    return

To produce Listing 8, I first highlighted lines 35-54 in Listing 7 and then used the Extract Method wizard to move them into the doString() method.

Listing 8. Moving Code with the Extract Method Wizard

""" TokenHandler readies tokens to passing to metric routines.

  $Id: tokenhandler.py,v 1.2 2005/01/19 06:13:33 rcharney Exp $
"""
__version__ = "$Revision: 1.2 $"[11:-2]

#import token
from globals import *

class TokenHandler( object ):
  """ Class to handle processing of tokens."""
  def __call__( self, tokCount, tok ):
    """ Common code for handling tokens."""
    tokCount += 1
    self.metrics['tokenCount'] = tokCount
    if not tok.type in [WS,INDENT,DEDENT,EMPTY,ENDMARKER]:
      prevTok = self.saveTok
      self.saveTok = tok
    self.context['blockNum'] = self.metrics.get('blockCount',0)
    self.context['blockDepth'] = self.metrics.get('blockDepth',0)
    self.context['parenDepth'] = self.parenDepth
    self.context['bracketDepth'] = self.bracketDepth
    self.context['braceDepth'] = self.braceDepth

    self.classDepth += self.classDepthIncr
    self.context['classDepth'] = self.classDepth
    self.classDepthIncr = 0
    
    self.fcnDepth += self.fcnDepthIncr    # incr at start of fcn body
    self.context['fcnDepth'] = self.fcnDepth
    self.fcnDepthIncr = 0
    
    # start testing for types that change in context
    
    self.doStrings(tok, prevTok)
      
    if self.defFunction:
      if tok.type == NAME:
        self.__incr( 'numFunctions')
        self.fqnName.append( tok.text )
        self.currentFcn = self.__genFQN( self.fqnName )
        self.fqnFunction.append( (self.currentFcn,self.blockDepth,FCNNAME) )
        self.defFunction = False
        self.findFcnHdrEnd = True
        if self.pa.verbose > 0: print "fqnFunction=%s" % self.fqnFunction
        self.__postToken( tok )
        return
      elif tok.type == ERRORTOKEN:
        # we must compensate for the simple scanner mishandling errors
        self.findFcnHdrEnd = True
        self.invalidToken = mytoken.MyToken(type=NAME, 
                                            semtype=FCNNAME, 
                                            text=tok.text, 
                                            row=tok.row, 
                                            col=tok.col, 
                                            line=tok.line)
        self.skipUntil = '(:\n'
        return
    elif self.defClass and tok.type == NAME:
      self.__incr( 'numClasses' )
      self.fqnName.append( tok.text )
      self.currentClass = self.__genFQN( self.fqnName )
      self.fqnClass.append( (self.currentClass,self.blockDepth,CLASSNAME) )
      self.defClass = False
      self.findClassHdrEnd = True
      if self.pa.verbose > 0: print "fqnClass=%s" % self.fqnClass
      self.__postToken( tok )
      return
      
    # return with types that don't change in context
    
    self.__postToken( tok )
    if tok.type == EMPTY:
      self.endOfStmt()
      return
    if tok.type == WS:
      return
    if tok.type == NEWLINE:
      self.endOfStmt()
      return
    
    # at this point, we have encountered a non-white space token
    # if a module doc string has not been found yet, it never will be.
    numModDocStrings = self.metrics['numModuleDocStrings']
    if self.checkForModuleDocString and numModDocStrings == 0:
      self.checkForModuleDocString = False
      msg = ( "Module %s is missing a module doc string. Detected at line %d" 
              % (self.context['inFile'],tok.row) )
      if not self.pa.quietSw:
        sys.stderr.writelines( msg )

    if tok.type == OP and tok.text == ':':
      if self.findFcnHdrEnd:
        self.findFcnHdrEnd = False
        self.docString = True
        self.fcnDepthIncr = 1
        return
      elif self.findClassHdrEnd:
        self.findClassHdrEnd = False
        self.docString = True
        self.classDepthIncr = 1
        return
    if tok.type == OP:
      if tok.text == '(':
        self.parenDepth += 1
      elif tok.text == ')':
        self.parenDepth -= 1
      elif tok.text == '[':
        self.bracketDepth += 1
      elif tok.text == ']':
        self.bracketDepth -= 1
      elif tok.text == '{':
        self.braceDepth += 1
      elif tok.text == '}':
        self.braceDepth -= 1
      return
        
    if tok.type == INDENT:
      self.__incr( 'blockCount')
      self.blockDepth += 1
      self.metrics['blockDepth'] = self.blockDepth
      if self.metrics.get('maxBlockDepth',0) < self.blockDepth:
        self.metrics['maxBlockDepth'] = self.blockDepth
      return
    elif tok.type == DEDENT:
      assert self.fcnDepth == len( self.fqnFunction )
      self.blockDepth -= 1
      self.metrics['blockDepth'] = self.blockDepth
      while len(self.fqnFunction) and self.blockDepth<=self.fqnFunction[-1][1]:
        msg = self.endOfFunction( self.fcnExits )
        if msg and not self.pa.quietSw:
          print msg
        if self.pa.verbose > 0: 
          print "Removing function %s" % self.__extractFQN( self.fqnFunction )
        del self.fqnFunction[-1]
        del self.fqnName[-1]
        self.fcnDepth -= 1
      assert self.fcnDepth == len( self.fqnFunction )
      if len( self.fqnFunction ) > 0:
        self.currentFcn = self.fqnFunction[-1][0]
      else:
        self.currentFcn = None
      while len(self.fqnClass) and self.blockDepth <= self.fqnClass[-1][1]:
        self.endOfClass()
        self.classDepth -= 1
        if self.pa.verbose > 0: 
          print "Removing class %s" % self.__extractFQN( self.fqnClass )
        del self.fqnClass[-1]
        del self.fqnName[-1]
      if len( self.fqnClass ) > 0:
        self.currentClass = self.fqnClass[-1][0]
      else:
        self.currentClass = None
      return

    self.docString = False
    if tok.semtype == KEYWORD:
      self.__incr( 'numKeywords')
      if tok.text == 'def':
        self.defFunction = True
      elif tok.text == 'class':
        self.defClass = True
      elif tok.text == 'return':
        assert self.fcnDepth == len( self.fqnFunction )
        if self.fcnDepth == 0:     # then we are not in any function
          if not self.pa.quietSw:  # then report on these types of errors
            msg = ( "Module %s contains the return statement at line %d " +
                    "that is outside any function" 
                    % (self.context['inFile'],tok.row) )
        if prevTok.text == ':': 
          # this return on same line as conditional, 
          # so it must be an extra return
          self.fcnExits.append( tok.row )
        elif self.blockDepth > 1:
          # Let fcnBlockDepth be the block depth of the function body.
          # We are trying to count the number of return statements
          # in this function. Only one is allowed at the fcnBlockDepth for the
          # function. If the self.blockDepth is greater than fcnBlockDepth, 
          # then this is a conditional return - i.e., an additional return
          fcnBlockDepth = self.fqnFunction[-1][1] + 1
          if self.blockDepth > fcnBlockDepth:
            self.fcnExits.append( tok.row )
            
    return

  def doStrings(self, tok, prevTok):
    if self.docString and tok.type == token.STRING:
      self.docString = False
      tok.semtype = DOCSTRING
      if self.checkForModuleDocString:  # we have found the module's doc string
        self.metrics['numModuleDocStrings'] += 1
        self.checkForModuleDocString = False
      self.__postToken( tok )
      return
    if tok.type == COMMENT:
      # compensate for older tokenize including newline
      # obals......in token when only thing on line is comment
      # this patch makes all connents consistent
      self.metrics['numComments'] += 1
      if tok.text[-1] == '\n':
        tok.text = tok.text[:-1]
      if prevTok and prevTok.type != NEWLINE and prevTok.type != EMPTY:
        tok.semtype = INLINE
        self.metrics['numCommentsInline'] += 1
      self.__postToken( tok )
      return

Notice that BRM is not as clever as Eclipse. It did not detect the use of return statements; it simply moved them into the extracted method. This would cause a problem when running, because the return would be returning from the extracted method--not from the original __call__ method. I fixed this manually by modifying the new method to return True if one of its returns was used; otherwise, it returned False. The __call__ method then tested the return value and exited itself if True. I also realized that the method name doStrings was a slight misnomer because the method also handled comments. So, I changed the method name to doStringsOrComments using BRM's Rename feature. Listing 9 shows these changes.

Listing 9. Renaming with BRM's Rename Feature

""" TokenHandler readies tokens to passing to metric routines.

  $Id: tokenhandler.py,v 1.2 2005/01/19 06:13:33 rcharney Exp $
"""
__version__ = "$Revision: 1.2 $"[11:-2]

#import token
from globals import *

class TokenHandler( object ):
  """ Class to handle processing of tokens."""
  def __call__( self, tokCount, tok ):
    """ Common code for handling tokens."""
    tokCount += 1
    self.metrics['tokenCount'] = tokCount
    if not tok.type in [WS,INDENT,DEDENT,EMPTY,ENDMARKER]:
      prevTok = self.saveTok
      self.saveTok = tok
    self.context['blockNum'] = self.metrics.get('blockCount',0)
    self.context['blockDepth'] = self.metrics.get('blockDepth',0)
    self.context['parenDepth'] = self.parenDepth
    self.context['bracketDepth'] = self.bracketDepth
    self.context['braceDepth'] = self.braceDepth

    self.classDepth += self.classDepthIncr
    self.context['classDepth'] = self.classDepth
    self.classDepthIncr = 0
    
    self.fcnDepth += self.fcnDepthIncr    # incr at start of fcn body
    self.context['fcnDepth'] = self.fcnDepth
    self.fcnDepthIncr = 0
    
    # start testing for types that change in context
    
    if self.doStringsOrComments(tok, prevTok):
      return
      
    if self.doFunctionName(tok):
      return      
    elif self.doClassName(tok):
      return
      
    # return with types that don't change in context
    
    self.__postToken( tok )
    if tok.type == EMPTY:
      self.endOfStmt()
      return
    if tok.type == WS:
      return
    if tok.type == NEWLINE:
      self.endOfStmt()
      return
    
    # at this point, we have encountered a non-white space token
    # if a module doc string has not been found yet, it never will be.
    numModDocStrings = self.metrics['numModuleDocStrings']
    if self.checkForModuleDocString and numModDocStrings == 0:
      self.checkForModuleDocString = False
      msg = ( "Module %s is missing a module doc string. Detected at line %d" 
              % (self.context['inFile'],tok.row) )
      print msg
      if not self.pa.quietSw:
        sys.stderr.writelines( msg )

    if tok.type == OP and tok.text == ':':
      if self.findFcnHdrEnd:
        self.findFcnHdrEnd = False
        self.docString = True
        self.fcnDepthIncr = 1
        return
      elif self.findClassHdrEnd:
        self.findClassHdrEnd = False
        self.docString = True
        self.classDepthIncr = 1
        return
    if tok.type == OP:
      if tok.text == '(':
        self.parenDepth += 1
      elif tok.text == ')':
        self.parenDepth -= 1
      elif tok.text == '[':
        self.bracketDepth += 1
      elif tok.text == ']':
        self.bracketDepth -= 1
      elif tok.text == '{':
        self.braceDepth += 1
      elif tok.text == '}':
        self.braceDepth -= 1
      return
        
    if tok.type == INDENT:
      self.__incr( 'blockCount')
      self.blockDepth += 1
      self.metrics['blockDepth'] = self.blockDepth
      if self.metrics.get('maxBlockDepth',0) < self.blockDepth:
        self.metrics['maxBlockDepth'] = self.blockDepth
      return
    elif tok.type == DEDENT:
      assert self.fcnDepth == len( self.fqnFunction )
      self.blockDepth -= 1
      self.metrics['blockDepth'] = self.blockDepth
      while len(self.fqnFunction) and self.blockDepth<=self.fqnFunction[-1][1]:
        msg = self.endOfFunction( self.fcnExits )
        if msg and not self.pa.quietSw:
          print msg
        if self.pa.verbose > 0: 
          print "Removing function %s" % self.__extractFQN( self.fqnFunction )
        del self.fqnFunction[-1]
        del self.fqnName[-1]
        self.fcnDepth -= 1
      assert self.fcnDepth == len( self.fqnFunction )
      if len( self.fqnFunction ) > 0:
        self.currentFcn = self.fqnFunction[-1][0]
      else:
        self.currentFcn = None
      while len(self.fqnClass) and self.blockDepth <= self.fqnClass[-1][1]:
        self.endOfClass()
        self.classDepth -= 1
        if self.pa.verbose > 0: 
          print "Removing class %s" % self.__extractFQN( self.fqnClass )
        del self.fqnClass[-1]
        del self.fqnName[-1]
      if len( self.fqnClass ) > 0:
        self.currentClass = self.fqnClass[-1][0]
      else:
        self.currentClass = None
      return

    self.docString = False
    if tok.semtype == KEYWORD:
      self.__incr( 'numKeywords')
      if tok.text == 'def':
        self.defFunction = True
      elif tok.text == 'class':
        self.defClass = True
      elif tok.text == 'return':
        assert self.fcnDepth == len( self.fqnFunction )
        if self.fcnDepth == 0:     # then we are not in any function
          if not self.pa.quietSw:  # then report on these types of errors
            msg = ( "Module %s contains the return statement at line %d " +
                    "that is outside any function" % 
                    (self.context['inFile'],tok.row) )
        if prevTok.text == ':': 
          # this return on same line as conditional, 
          # so it must be an extra return
          self.fcnExits.append( tok.row )
        elif self.blockDepth > 1:
          # Let fcnBlockDepth be the block depth of the function body.
          # We are trying to count the number of return statements
          # in this function. Only one is allowed at the fcnBlockDepth for the
          # function. If the self.blockDepth is greater than fcnBlockDepth, 
          # then this is a conditional return - i.e., an additional return
          fcnBlockDepth = self.fqnFunction[-1][1] + 1
          if self.blockDepth > fcnBlockDepth:
            self.fcnExits.append( tok.row )
            
    return

  def doClassName(self, tok):
    """ Return True if current token is class name."""
    if self.defClass and tok.type == NAME:
      self.__incr( 'numClasses' )
      self.fqnName.append( tok.text )
      self.currentClass = self.__genFQN( self.fqnName )
      self.fqnClass.append( (self.currentClass,self.blockDepth,CLASSNAME) )
      self.defClass = False
      self.findClassHdrEnd = True
      if self.pa.verbose > 0: print "fqnClass=%s" % self.fqnClass
      self.__postToken( tok )
      return True
    return False

  def doFunctionName(self, tok):
    """ Return True if this is a function name."""
    if self.defFunction:
      if tok.type == NAME:
        self.__incr( 'numFunctions')
        self.fqnName.append( tok.text )
        self.currentFcn = self.__genFQN( self.fqnName )
        self.fqnFunction.append( (self.currentFcn,self.blockDepth,FCNNAME) )
        self.defFunction = False
        self.findFcnHdrEnd = True
        if self.pa.verbose > 0: print "fqnFunction=%s" % self.fqnFunction
        self.__postToken( tok )
        return True
      elif tok.type == ERRORTOKEN:
        # we must compensate for the simple scanner mishandling errors
        self.findFcnHdrEnd = True
        self.invalidToken = mytoken.MyToken(type=NAME, 
                                            semtype=FCNNAME, 
                                            text=tok.text, 
                                            row=tok.row, 
                                            col=tok.col, 
                                            line=tok.line)
        self.skipUntil = '(:\n'
        return True
      return False

  def doStringsOrComments(self, tok, prevTok):
    """ Return True if this is a docstring or comment."""
    if self.docString and tok.type == token.STRING:
      self.docString = False
      tok.semtype = DOCSTRING
      if self.checkForModuleDocString:  # we have found the module's doc string
        self.metrics['numModuleDocStrings'] += 1
        self.checkForModuleDocString = False
      self.__postToken( tok )
      return True
    if tok.type == COMMENT:
      # compensate for older tokenize including newline
      # obals......in token when only thing on line is comment
      # this patch makes all connents consistent
      self.metrics['numComments'] += 1
      if tok.text[-1] == '\n':
        tok.text = tok.text[:-1]
      if prevTok and prevTok.type != NEWLINE and prevTok.type != EMPTY:
        tok.semtype = INLINE
        self.metrics['numCommentsInline'] += 1
      self.__postToken( tok )
      return True
    return False

An early refactoring program, BRM has a number of other issues of which you should be aware. They include:

Far more refactoring can be done on both the Java and Python code samples. But, I believe I have given you a brief overview of what current refactoring tools can do.

The benefits of using tools to do code refactoring include:

I realize I haven't discussed why you should refactor your code, or anyone else's for that matter. Often, there are both technical and managerial reasons to do refactoring. Using prototypes for production purposes is one of the bugaboos of programming, and we know the problems this causes. However, refactoring may be a way out of this cul-de-sac. Perhaps, you can use the benefits of refactoring to justify a rewrite to management.

I am puzzled by the differences in the interest between refactoring and metrics. I have found no objective way of measuring the effect of code refactoring. It seems that we simply know that the refactored code is "better".

I am also bothered by my response to one comment on last month's column. The writer mentioned that Function Points are an accepted metric for measuring complexity. They also provide good, consistent metrics for estimating project costs. I made an error of omission in not mentioning function points. On further reflection, I still don't see them as effective code metrics. Function points most often are used during specification and design of systems. They also are labor-intensive. I know of no product out there that can do a proper function point analysis on an existing code base. Please correct me if I am wrong.

Sources for Information about Refactoring

Open Source Plugin for jEdit

An Older Java Refactoring Tool

Java freeware, usable as standalone or plugin for a number of editors and IDEs; recent and extensive.

Pymetrics Home Page

Refactoring: Improving the Design of Existing Code by Martin Fowler, Don Roberts, John Brant, William Opdyke (ISBN 0-201-48567-2), Addison-Wesley.

Refactoring to Patterns by Joshua Kerievsky, ISBN 0-321-21335-1, Addison-Wesley