"Replace Type Code with Subclass" in Smaller Steps

Last evening (4/17/2006) I was giving a presentation on the Refactoring "Move Embellishment to Decorator" and I received a challenge from Brian Button to do the "Replace Type Code with Subclass" portion of the Refactoring in smaller steps. Never one to turn down a good challenge I promised to do the Refactoring in smaller steps and blog it. True to my word I have accomplished the smaller steps and this article will detail what I did.

First it is important to point out that the code being refactored is sufficiently covered with tests (about 96% covered). This is very important for me because it gives me the courage to do these refactorings without the fear of changing the intent of the code and breaking code in seemingly unrelated areas.

Warning: This article is quite lengthy due to all of the source code included.

Note: Blogger reformatted the code some so it is not properly indented. This code was created solely for the purpose of this demonstration, and yes I am addicted to MMORPG games like World of Warcraft.

When I did this Refactoring in the live demonstration I converted all three of the weapon types to subclasses and implemented a static factory method to new them up, pushing the conditional logic into each subclass. However, during this time the code was broken and I could not even run the tests until I completed the creation of all three subclasses [not a good thing]. The steps outlined below allow you to do the same Refactoring but with working code all the way.

You can download the code here if you want to follow along on your own.

Here is what the class looked like when it was using type codes to determine what conditional logic to execute based on the type of weapon that we wanted to create. The constructor takes a Rollable and an int representing the type of weapon being used. The attack() method then performs the appropriate attack based on the weapon type.

public class MeleeWeapon implements Weapon{
   private Rollable dice;
      private int weaponType = INVALID_MELEE_WEAPON;
   // Publically Available Weapons
   public static final int KNOCK_DOWN_MELEE_WEAPON = 3;
      public static final int STUNNING_MELEE_WEAPON = 2;
      public static final int NORMAL_MELEE_WEAPON = 1;
      public static final int INVALID_MELEE_WEAPON = 0;

   //Private Constants
      private static final int DAMAGE_AMOUNT_REQUIRED_TO_STUN = 10;
      private static final int STUN_DURATION = 5;
      private static final int KNOCK_DOWN_DURATION = 3;

   public MeleeWeapon(Rollable dice, int weaponType) {
             this.dice = dice;
             this.weaponType = weaponType;   }

   public void attack(Player playerToAttack) {
             int damageAmount = dice.roll();
      playerToAttack.scoreDamage(damageAmount);
      switch (this.weaponType){
         case NORMAL_MELEE_WEAPON:{
            //no special behavior
            break;
         }
         case STUNNING_MELEE_WEAPON:{
           if (damageAmount >= DAMAGE_AMOUNT_REQUIRED_TO_STUN) {
              playerToAttack.stun(STUN_DURATION);
           }
           break;
         }
         case KNOCK_DOWN_MELEE_WEAPON: {
           playerToAttack.knockDown(KNOCK_DOWN_DURATION);
           break;
         }
      }
   }
}

Step 1:
Added the following three methods and made the old constructor private to keep any client code from newing up the object without using the factory method:

    public int getWeaponType() {
   return weaponType;
    }

    public MeleeWeapon(Rollable dice){
   this.dice = dice;
    }
    
    public static MeleeWeapon create(Rollable dice, int weaponType){
   return new MeleeWeapon(dice, weaponType);
    }

and changed the client code (my unit tests) to use the static factory method.
Before change:

new MeleeWeapon(dice, MeleeWeapon.NORMAL_MELEE_WEAPON)

After change:

MeleeWeapon.create(dice, MeleeWeapon.NORMAL_MELEE_WEAPON)

Compile and Test

Step2:
Create the first subclass and modify the factory method appropriately and change the MeleeWeapon class to use the new subclass.

MeleeWeapon class changes:

    public static MeleeWeapon create(Rollable dice, int weaponType){
   if (weaponType == NORMAL_MELEE_WEAPON) return new NormalMeleeWeapon(dice);
   else return new MeleeWeapon(dice, weaponType);
    }

Remove the NormalMeleeWeapon leg from the attack() switch statement and push this behavior into the new subclass. You will also have to make the dice field in MeleeWeapon protected.

NormalMeleeWeapon subclass:

public class NormalMeleeWeapon extends MeleeWeapon {
   public NormalMeleeWeapon(Rollable dice) {
      super(dice);
   }

   public int getWeaponType() {
      return MeleeWeapon.NORMAL_MELEE_WEAPON;
   }

   public void attack(Player playerToAttack) {
      int damageAmount = dice.roll();
      playerToAttack.scoreDamage(damageAmount);
   }
}

Compile and Test

Repeat Step 2 until all the subclasses have been created.

Step 3:
Remove the attack() method from the MeleeWeapon class.

Refactor the static factory method to look like this:

public static MeleeWeapon create(Rollable dice, int weaponType){
   switch (weaponType){
      case NORMAL_MELEE_WEAPON:
         return new NormalMeleeWeapon(dice);
      case STUNNING_MELEE_WEAPON:
         return new StunningMeleeWeapon(dice);
      case KNOCK_DOWN_MELEE_WEAPON:
         return new KnockDownMeleeWeapon(dice);
      default:
         throw new IllegalArgumentException("Invalid weapon type");
   }
    }

Remove private constructor for the MeleeWeapon class and make the class abstract.
Remove the implementation from the getWeaponType method in MeleeWeapon and make it abstract.

Compile and Test

You Are Done!

Ok all the tests are green and we were able to keep them green through these smaller steps. The main difference that I noted when doing the Refactoring this way was the feeling of comfort after creating each new subclass and seeing the tests pass in step 2. Previously, as I mentioned earlier, I had to do a bigger chunk of work and then pray that I hadn’t forgotten anything and that the tests would pass. These smaller steps felt more comfortable and the outcome was the same.

Please note that in the end we still have a switch statement, however the switch is used for object creation which is better than using it to determine the special logic to execute in the attack() method based on the weaponType.

I’m glad Brian challenged me on this. Having now done this Refactoring both ways I think these smaller steps are really the way to go. Thanks Brian.

What do you think? Better? Worse? Neither? Send me a comment and let me know.