Method-Level Refactorings

Moving a method or field

Problem

A method may have made sense in one class but now seems to make more sense in another. This sometimes manifests itself as Feature Envy: a method habitually needs access to the data of another object:

Before

After

Method

Let's start with a simple example. In the interests of separating data from presentation, a BoxView is associated with a Box, which it displays:

class Box {
   private int height, width, length;
   public Box(int height, int width, int length) {
      this.height = height;
      this.width = width;
      this.length = length;
   }
   public int getHeight() { return height; }
   public int getWidth() { return width; }
   public int getLength() { return length; }
}

Here's the BoxView class:

public class BoxView {
   private Box myBox;
   public void setBox(Box box) { myBox = box; }
   public int getVolume() {
      return myBox.getHeight() *
               myBox.getWidth() * myBox.getLength();
   }
   public void display() {
      System.out.println("Box View: ");
      System.out.println(
         "height = " + myBox.getHeight() + " inches");
      System.out.println(
         "width = " + myBox.getWidth() + " inches");
      System.out.println(
         "length = " + myBox.getLength() + " inches");
      System.out.println(
         "volume = " + getVolume() + " cubic inches");
   }
}

Declaring the getVolume() method in the BoxView class may have seemed like a good idea at one time, but it clearly ought to the in the Box class now.

Step 1

Terminology: we refer to BoxView as the source class, BoxView.getVolume() as the source method, Box as the destination class, and BoxView.getVolume() as the destination method.

Copy the source method to the target class and adjust the implementation. Replace the implementation of the source method with a call to the target method. Build and test.

class Box {
   public int getVolume() {
      return height * width * length;
   }
   // etc.
}

public class BoxView {
   private Box myBox;
   public void setBox(Box box) { myBox = box; }
   public int getVolume() {
      return myBox.getVolume();
   }
   // etc.
}

Step 2

Remove the source method. Modify all calls to the source method:

public class BoxView {
   private Box myBox;
   public void setBox(Box box) { myBox = box; }
   public void display() {
      System.out.println("Box View: ");
      System.out.println(
         "height = " + myBox.getHeight() + " inches");
      System.out.println(
         "width = " + myBox.getWidth() + " inches");
      System.out.println(
         "length = " + myBox.getLength() + " inches");
      System.out.println(
         "volume = " + myBox.getVolume() + " cubic inches");
   }
}

Accessing source class methods and fields

Here's a slightly more complicated example. We add a getCost() method to our Box class. The cost of the box depends on the amount of material (surface area) as well as a description of the box:

class Box {
   private static double unitCostPine = .03;
   private static double unitCostOak = .04;
   private static double unitCostCardboard = .02;
   private BoxDescription description;
   public void setBoxDescription(BoxDescription desc) {
      description = desc;
   }
   public BoxDescription getDescription() {
      return dewcription;
   }
   public double getCost() {
      int surfaceArea =
         2 * (height * (width + length) + width * length);
      switch (description.getMaterial()) {
         case BoxDescription.CARDBOARD:
            return unitCostCardboard * surfaceArea;
         case BoxDescription.PINE:
            return unitCostPine * surfaceArea;
         case BoxDescription.OAK:
            return unitCostOak * surfaceArea;
         default: return 0;
      }
   }
   // etc.
}

A box description might include a description of the type of material the box is made from:

class BoxDescription {
   public final static int PINE = 0;
   public final static int OAK = 1;
   public final static int CARDBOARD = 2;
   private int material;
   public BoxDescription(int mat) { material = mat; }
   public int getMaterial() { return material; }
   // etc.
}

Let's move the getCost() method to the BoxDescription class. Unfortunately, the method it replaces needs information from the Box class. How shall this information be made available to the BoxDescription class? There are four techniques:

1. Pass a reference to the source object as a parameter.
2. Provide the target class with a field that references the source class object.
3. Move the needed fields into the target class.
4. Pass the needed information as parameters.

We combine techniques 1 and 3:

class BoxDescription {
   public final static int PINE = 0;
   public final static int OAK = 1;
   public final static int CARDBOARD = 2;
   private static double unitCostPine = .03;
   private static double unitCostOak = .04;
   private static double unitCostCardboard = .02;
   private int material;
   public BoxDescription(int mat) { material = mat; }
   public int getMaterial() { return material; }
   public double getCost(Box box) {
      int height = box.getHeight();
      int width = box.getWidth();
      int length = box.getLength();
      int surfaceArea =
         2 * (height * (width + length) + width * length);
      switch (getMaterial()) {
         case BoxDescription.CARDBOARD:
            return unitCostCardboard * surfaceArea;
         case BoxDescription.PINE:
            return unitCostPine * surfaceArea;
         case BoxDescription.OAK:
            return unitCostOak * surfaceArea;
         default: return 0;
      }
   }
}

Finally, we replace the implementation in the source class:

class Box {
   private BoxDescription description;
   public double getCost() {
      return description.getCost(this);
   }
   // etc.
}

Moving Fields

Fortunately, moving the unit cost fields to the target class wasn't problematic because they were only used by the moved method. In general, moving a field involves adding getters and setters in the target class, then replacing all references in the source class by calls to these methods.

For example, assume class Source has a field val that we would like to move to class Target:

class Source {
   private int val = 100;
   public Source(int v) { val = v; }
   int double() { return 2 * val++; }
   // etc.
}

Here's the class Target with its new field plus accompanying getter and setter methods:

class Target {
   private int val = 100;
   int getVal() { return val; }
   void setVal(int v) { val = v; }
   // etc.
}

We must provide the Source class with a reference to a Target class instance. We must also modify all references to val:

class Source {
   private Target someTarget = new SomeTarget();
   public Source(int v) { someTarget.setVal(v); }
   int double() {
      int val = someTarget.getVal();
      someTarget.setValue(value + 1);
      return val;
   }
   // etc.
}

Dealing with polymorphism

Moving a method may not be possible if the method is also declared in subclasses or superclasses. This depends on if the polymorphism can be moved to the target class as well. For example, assume a FancyBox subclass of Box overrides the inherited getCost() method using different unit cost factors:

class FancyBox extends Box {
   private static double unitCostPine = .035;
   private static double unitCostOak = .41;
   private static double unitCostCardboard = .025;
   public FancyBox(int height, int width, int length) {
      super(height, width, length);
   }
   public double getCost() {
      int height = getHeight();
      int width = getWidth();
      int length = getLength();
      int surfaceArea =
         2 * (height * (width + length) + width * length);
      switch (getDescription().getMaterial()) {
         case BoxDescription.CARDBOARD:
            return unitCostCardboard * surfaceArea;
         case BoxDescription.PINE:
            return unitCostPine * surfaceArea;
         case BoxDescription.OAK:
            return unitCostOak * surfaceArea;
         default: return 0;
      }
   }
}

Unfortunately, moving the dispatch on box type to the BoxDescription class forces us to make the dispatch explicit:

class BoxDescription {
   private static double unitCostCardboard = .02;
   private static double unitCostCardboard2 = .025;
   // etc.
   public double getCost(Box box) {
      int height = box.getHeight();
      int width = box.getWidth();
      int length = box.getLength();
      int surfaceArea =
         2 * (height * (width + length) + width * length);
      String type = box.getClass().getName();
      switch (getMaterial()) {
         case BoxDescription.CARDBOARD:{
            if (type.equals("FancyBox")) {
               return unitCostCardboard * surfaceArea;
            } else {
               return unitCostCardboard2 * surfaceArea;
            }
         }
         // etc.
      }
   }
}

Renaming a method

Problem

The name of a method doesn't reflect its purpose

public class Student {
   public int calc(int exam1, int exam2, int exam3) {
      return (exam1 + exam2 + exam3)/3;
   }
   // etc.
}

public class StudentTest extends TestCase {
   Student s1, s2;
   protected void setUp() { s1 = new Student(); }
   public void testAvg() {
      assertEquals(s1.calc(70, 80, 90), 80);
   }
   public static Test suite() {
      return new TestSuite(StudentTest.class);
   }
}

Method

Step 1

Make a copy the old method and rename it. Change the body of the old method so that it calls the new method:

public class Student {
   public int calc(int exam1, int exam2, int exam3) {
      return avgScore(exam1, exam2, exam3);
   }
   public int avgScore(int exam1, int exam2, int exam3) {
      return (exam1 + exam2 + exam3)/3;
   }
   // etc.
}

Assume this method is also implemented in a subclass or superclass:

public class GradStudent extends Student {
   private boolean onProbation = false;
   public int calc(int exam1, int exam2, int exam3) {
      int avg = (exam1 + exam2 + exam3)/3;
      if (avg < 80) onProbation = true;
      return avg;
   }
   // etc.
}

Subsumption allows GradStudent references to appear in contexts where Student references are expected:

Student s2 = new GradStudent();

Dynamic dispatch allows the object (not the compiler) to determine which method should be called:

s2.calc(70, 80, 90); // calls GradStudent.calc()

Step 2

Repeat the renaming procedure for each of these subclasses and superclasses:

public class GradStudent extends Student {
   boolean onProbation = false;
   public int calc(int exam1, int exam2, int exam3) {
      return avgScore(exam1, exam2, exam3);
   }
   public int avgScore(int exam1, int exam2, int exam3) {
      int avg = (exam1 + exam2 + exam3)/3;
      if (avg < 80) onProbation = true;
      return avg;
   }
   // etc.
}

Step 3

Build and test.

Step 4

Find all calls to the old method and change them to calls to the new method.

   public void testAvg() {
      assertEquals(s1.avgScore(70, 80, 90), 80);
   }

Step 5

Remove the declaration of the old method or mark it as deprecated.

Step 6

Build and test.

Adding parameters

Problem

It can be difficult to anticipate all of the parameters needed by a method

Method

Step 1

Declare the new method with the new parameters. Thankfully, overloading allows us to do this without making up a new name for the new method:

public class Student {
   public int avgScore(int exam1, int exam2, int exam3) {
      return (exam1 + exam2 + exam3)/3;
   }
   public int avgScore(int exam1, int exam2, int exam3, int exam4) {
      return (exam1 + exam2 + exam3 + exam4)/4;
   }
   // etc.
}

Step 2

Repeat for all subclasses and superclasses:

public class GradStudent extends Student {
   boolean onProbation = false;
public int avgScore(int exam1, int exam2, int exam3) {
      int avg = (exam1 + exam2 + exam3)/3;
      if (avg < 80) onProbation = true;
      return avg;
   }
   public int avgScore(int exam1, int exam2, int exam3, int exam4) {
      int avg = (exam1 + exam2 + exam3 + exam4)/4;
      if (avg < 80) onProbation = true;
      return avg;
   }
   // etc.
}

Step 3

Build.

Step 4

Change the body of the old method so that it calls the new method. Use a null or clearly bogus value for the new argument:

public class Student {
   public int avgScore(int exam1, int exam2, int exam3) {
      return avgScore(exam1, exam2, exam3, -100);
   }
   public int avgScore(int exam1, int exam2, int exam3, int exam4) {
      return (exam1 + exam2 + exam3 + exam4)/4;
   }
   // etc.
}

Of course we need to change the expected result in our test case:

   public void testAvg() {
      assertEquals(s1.avgScore(70, 80, 90), 35);
   }

Step 5

Build and test.

Step 6

Find all calls to the old method and replace them with calls to the new method:

   public void testAvg() {
      assertEquals(s1.avgScore(70, 80, 90, 100), 85);
   }

Step 7

Remove the old method or mark it as deprecated.

Step 8

Build and test.

Removing parameters

Problem

Functions that have lots of obscure parameters are unpleasant to use.

Method

Removing a parameter is similar to adding a parameter. The old method calls the new method:

public class Student {
   public int avgScore(int exam1, int exam2) {
      return (exam1 + exam2)/2;
   }
   public int avgScore(int exam1, int exam2, int exam3, int exam4) {
      return avgScore(exam1, exam2);
   }
   // etc.
}

The procedure is repeated in subclasses and superclasses. After a build and test, calls to the old method are replaced by calls to the new method:

   public void testAvg() {
      assertEquals(s1.avgScore(80, 100), 90);
   }

The old method is removed:

public class Student {
   public int avgScore(int exam1, int exam2) {
      return (exam1 + exam2)/2;
   }
   // etc.
}

A final build and test is performed.

Introducing parameter objects

Problem

Often the parameters of an object are closely related

Method

Step 1

Declare and build a new immutable class to represent the group of parameters to be replaced:

public class Scores {
   private int exam1, exam2;
   public Scores() {
      exam1 = exam2 = 0;
   }
   public Scores(int exam1, int exam2) {
      this.exam1 = exam1;
      this.exam2 = exam2;
   }
   int getExam1() { return exam1; }
   int getExam2() { return exam2; }
}

Step 2

Following the method for adding parameters, we introduce a new parameter object parameter into the method:

Step 2.1: Introduce a new method containing the extra parameter, then build and test:

public class Student {
   public int avgScore(int exam1, int exam2) {
      return avgScore(exam1, exam2, new Scores(exam1, exam2));
   }
   public int avgScore(int exam1, int exam2, Scores scores) {
      return (scores.getExam1() + scores.getExam2())/2;
   }
   // etc.
}

Step 2.2: Replace calls to the old method by calls to the new method:

public void testAvg() {
assertEquals(s1.avgScore(70, 80, new Scores(70, 80)), 75);
}

Step 2.3: Remove or deprecate the declaration of the old method, then build and test:

public class Student {
   public int avgScore(int exam1, int exam2, Scores scores) {
      return (scores.getExam1() + scores.getExam2())/2;
   }
   // etc.
}

Step 3

Follow the method for removing parameters to remove the extra parameters.

Step 3.1: Add a new method without the old parameters. Replace the body of the old method with a call to the new method, then build and test:

public class Student {
   public int avgScore(int exam1, int exam2, Scores scores) {
      return avgScore(scores);
   }
   public int avgScore(Scores scores) {
      return (scores.getExam1() + scores.getExam2())/2;
   }
   // etc.
}

Step 3.2: Replace calls to the old method by calls to the new method, remove the old method, then build and test again:

   public void testAvg() {
      assertEquals(s1.avgScore(new Scores(70, 80)), 75);
   }

Step 4

Use the Move Method procedure to move any behavior to the parameter object.

In this case we probably should move the avgScore method to the Scores class:

public class Student {
   public int avgScore(Scores scores) {
      return scores.avgScore();
   }
   // etc.
}

public class GradStudent extends Student {
   boolean onProbation = false;
   public int avgScore(Scores scores) {
      int avg = scores.avgScore();
      if (avg < 80) onProbation = true;
      return avg;
   }
   // etc.
}

Replace constructor with factory method

Problem

Type tags do have their place in situations where the types and rules are shifting dynamically and inheritance and subsumption aren't issues. But type tags do lead to lots of switch statements that must all be updated when new types are added to the system.

Procedure

Replace type tags with polymorphism or reflection.

Example

Type tags are used to distinguish different types of employees:

The type tags are used in a client class:

public class Company {
   public final static int CAP = 100;
   private Employee[] staff = new Employee[CAP];
   private int staffSize = 0;
   public void hire(Employee e) {
      if (staffSize < CAP) staff[staffSize++] = e;
   }
   public int countPosition(String type) {
      int total = 0;
      for(int i = 0; i < staffSize; i++) {
         if (staff[i].getType().equals(type)) total++;
      }
      return total;
   }
}

Step 1

Make the constructor protected or private. Introduce a public static factory method:

Step 2

Introduce subclasses for the different types of employees:

class Programmer extends Employee {
   public Programmer() {
      super(Employee.PROGRAMMER);
   }
   // etc.
}

Replace the constructor calls in the factory method with calls to subclass constructors:

   public static Employee makeEmployee(String type) {
      if (type.equals(PROGRAMMER)) return new Programmer();
      if (type.equals(MANAGER)) return new Manager();
      if (type.equals(SECRETARY)) return new Secretary();
      return new Employee(UNKNOWN);
   }

Step 3

Using reflection we can replace the getType() method by the inherited getClass() method:

class Employee {
   public static final String UNKNOWN = "Unknown";
   public static final String PROGRAMMER = "Programmer";
   public static final String MANAGER = "Manager";
   public static final String SECRETARY = "Secretary";
   //private String type;
   public String getType() { return getClass().getName(); }
   protected Employee(String type) { this.type = type; }
   protected Employee() { }
   public static Employee makeEmployee(String type) {
      Employee result = null;
      try {
         result = (Employee)Class.forName(type).newInstance();
      } catch(Exception e) {
         System.err.println("unrecognized employee type: " + type);
         System.exit(1);
      }
      return result;
   }
   // etc.
}

Note: Employee type names should be fully qualified:

public static final String PROGRAMMER = "business.Programmer";

Note: We should be able to remove the type names altogether.

Removing Setters

Problem

A field needs to be made immutable

public class Person {
   private String name;
   public String getName() { return name; }
   public void setName(String name) { this.name = name; }
   public Person(String name) {
      this.name = name;
   }
   // etc.
}

Procedure

public class Person {
   final private String name;
   public String getName() { return name; }
   // public void setName(String name) { this.name = name; }
   public Person(String name) {
      this.name = name;
   }
   // etc.
}

Replacing error handling code with an exception

Problem

An antiquated or debug mode error handling strategy needs to be upgraded to a modern release mode strategy.

Procedure

Recall that Java exceptions can be checked or unchecked. (Checked exceptions must be declared in the throwing method's signature). Here's a brief synopsis of Java's Throwable hierarchy:

Throwable (checked)
   Error (unchecked)
      ...
   Exception (checked)
      ClassNotFoundException (checked)
      InterruptedException (checked)
      IOException (checked)
         ...
      RuntimeException (unchecked)
         ...

Step 1: Create an exception class. Decide if it should be checked or unchecked:

public class InvalidEmployeeType extends RuntimeException {
   public InvalidEmployeeType(String type) {
      super("invalid employee type: " + type);
   }
}

Step 2:

Replace the error code:

   public static Employee makeEmployee(String type)
   throws InvalidEmployeeType {
      Employee result = null;
      try {
         result = (Employee)Class.forName(type).newInstance();
      } catch(Exception e) {
         throw new InvalidEmployeeType(type);
      }
      return result;
   }

Step 3: Put calls in a try-catch block:

   protected void setUp() {
      try {
         comp.hire(Employee.makeEmployee(Employee.PROGRAMMER));
         comp.hire(Employee.makeEmployee(Employee.PROGRAMMER));
         comp.hire(Employee.makeEmployee(Employee.PROGRAMMER));
         comp.hire(Employee.makeEmployee(Employee.MANAGER));
      } catch(InvalidEmployeeType iet) {
         fail(iet.getMessage());
      }
   }

Replace Array with Object

Problem

The components of an array represent the attributes of an object.

Object[] person = new Object[3];
person[0] = "Smith";
person[1] = new Address(321, "Sesame St", "NYC");
person[2] = new PhoneNumber(213, 5551234);

Procedure

Replace the array with an object.

class Person {
   private String name;
   private Address address;
   private PhoneNumber phone;
   // etc.
}