Identifying and refactoring - Inversion Of Control

In my new position, I am now working on a large team of developers (15 in all). This team is working to complete a single product that has been designed using a Service Oriented Architecture. I have some insights into that design as well, however, in this post I thought I would show an example of how to identify and refactor to a design pattern, and why I feel this is a good thing for the project in the long term.

One of the hardest things about design patterns is recognizing when to use them and when not to. I have seen so many architects apply patterns for patterns sake, making code more complex, and making it HARDER to apply patterns where they are truly needed. I've also seen lots of code that isn't refactored to design patterns, and it COULD be made much easier to maintain and understand if it were changed. The code I will be talking about today is of the latter type. I've been writing Unit tests against the codebase at my new position. This has given me an opportunity to review the code. In order to protect the IP of the company I work for, the samples below have been changed. They represent the structure of some of the code, but are entirely unrelated to what my company does.

First, let me describe the general design of the code as it stands. In this instance, we have a method that takes an Enumeration argument. The method then calls the getInstance method of a Factory, and gets the proper implementation of an Interface. The getInstance method also takes an Enumeration. Finally, the code uses a case statement that tests the Enumeration value and uses the interface to set the values of different member variables in the class.  Here is the sample code that would implement this design:

The Container.  This is the class where our execution starts.  We call setMapValue and pass it a Type enum.

  1 package com.arciszewski.staterefactor; 
  2 import com.arciszewski.staterefactor.Type; 
  3 import java.util.Map; 
  4  
  5 public class Container {
  6     private Map<String,String> map1; 
  7     private Map<String,String> map2; 
  8     private Map<String,String> map3; 
  9      
 10     public void setMapValue(Type type) {
 11          
 12         ITypeInterface intf = TypeFactory.getInstance(type); 
 13          
 14         switch(type) { 
 15             case ONE : 
 16                 map1 = intf.getMap(); 
 17                 return; 
 18             case TWO :
 19                 map2 = intf.getMap(); 
 20                 return; 
 21             case THREE : 
 22                 map3 = intf.getMap(); 
 23                 return; 
 24             default : 
 25                 return; 
 26                  
 27                  
 28         } 
 29          
 30          
 31     } 
 32  
 33     public Map<String, String> getMap1() { 
 34         return map1; 
 35     } 
 36  
 37     public void setMap1(Map<String, String> map1) {
 38         this.map1 = map1; 
 39     } 
 40  
 41     public Map<String, String> getMap2() { 
 42         return map2; 
 43     } 
 44  
 45     public void setMap2(Map<String, String> map2) {
 46         this.map2 = map2; 
 47     } 
 48  
 49     public Map<String, String> getMap3() { 
 50         return map3; 
 51     } 
 52  
 53     public void setMap3(Map<String, String> map3) {
 54         this.map3 = map3; 
 55     } 
 56      
 57 } 
 58  
 59 

This is the Factory that creates ITypeIntf concrete classes.  For now there is only one implementation for type.ONE.

  1 package com.arciszewski.staterefactor; 
  2  
  3 public class TypeFactory {
  4  
  5     public static ITypeInterface getInstance(Type type) {
  6          
  7         ITypeInterface typeIntf = null; 
  8         if(type.ONE == type) { 
  9           return new  TypeImplOne();  
 10         } if(type.TWO == type) { 
 11              
 12         } if(type.THREE == type) { 
 13              
 14         } 
 15         return null; 
 16     } 
 17      
 18 }

The Enum.  Not much to say here.

  1 package com.arciszewski.staterefactor; 
  2  
  3 public enum Type { 
  4     ONE, TWO, THREE; 
  5 }

The Interface that defines getMap.

  1 package com.arciszewski.staterefactor; 
  2  
  3 import java.util.Map; 
  4  
  5 public interface ITypeInterface {
  6  
  7     public Map<String, String> getMap(); 
  8 }

An implementation class. Kept simple for discussion.

  1 package com.arciszewski.staterefactor; 
  2  
  3 import java.util.HashMap; 
  4 import java.util.Map; 
  5  
  6 public class TypeImplOne implements ITypeInterface {
  7  
  8     @Override 
  9     public Map<String, String> getMap() { 
 10         //Do something like get data from a DB. 
 11         return new HashMap<String, String>();
 12     } 
 13  
 14 }

Looking at this code, you'd probably say that the developers did some things right.  They designed to interfaces, they are using a creational pattern, and the code is pretty clean and easy to read.  I tend to agree, except when it comes to that ugly switch statement.  Here are my issues with the switch statement:

1. Developers have to remember the return statement, or we will have NULL implementations when we shouldn't
2. Developers will be tempted to add more logic to the case statements that are Type specific.  As these add up, the clarity of the code falls
3. We're already verifying the type in the Factory, so this code is checking the Type value twice when it doesn't have to.
4. Container needs to be aware of the different implementations of ITypeIntf.  Why should it need to?  Now we've added a dependency we don't need.

There is a change we can make that will improve this code.  Let's change the ITypeInterface to take a Container object in its argument list.  Then we can set the value in the method and the case statement is no longer needed.  This is a classic IOC technique.  Here is what the code looks like after the refactoring.

The new Container. Notice, there is no switch statement. None is needed. the IMPLEMENTATION of the ITypeIntf knows which attribute it needs to set. Also note that Container doesn't need to know anything about the implementation that the Factory returns.

  1 package com.arciszewski.staterefactor; 
  2 import com.arciszewski.staterefactor.Type; 
  3 import java.util.Map; 
  4  
  5 public class Container {
  6     private Map<String,String> map1; 
  7     private Map<String,String> map2; 
  8     private Map<String,String> map3; 
  9      
 10     public void setMapValue(Type type) {
 11         ITypeInterface intf = TypeFactory.getInstance(type); 
 12         intf.setMapValue(this); 
 13     } 
 14  
 15     public Map<String, String> getMap1() { 
 16         return map1; 
 17     } 
 18  
 19     public void setMap1(Map<String, String> map1) {
 20         this.map1 = map1; 
 21     } 
 22  
 23     public Map<String, String> getMap2() { 
 24         return map2; 
 25     } 
 26  
 27     public void setMap2(Map<String, String> map2) {
 28         this.map2 = map2; 
 29     } 
 30  
 31     public Map<String, String> getMap3() { 
 32         return map3; 
 33     } 
 34  
 35     public void setMap3(Map<String, String> map3) {
 36         this.map3 = map3; 
 37     } 
 38      
 39      
 40 }

The new interface. The interface definition was changed to return void and to make the name descriptive of what it does now.

  1 package com.arciszewski.staterefactor; 
  2  
  3 public interface ITypeInterface {
  4  
  5     public void setMapValue(Container container);
  6 }

The new implementation. Notice that the new implementation now has a dependency on Container. That's OK in my mind, since the interface is intended to work on Container. Here's what we gained, though. Now ALL the logic for this condition is in ONE place. Container will never need to be changed, even if a new implementation is defined (For instance, an implementation that will set ALL the map attribute values.)

  1 package com.arciszewski.staterefactor; 
  2  
  3 import java.util.HashMap; 
  4  
  5 public class TypeImplOne implements ITypeInterface {
  6     // Changed the name, too. getMap no longer describes this function, but
  7     // setMapValue does. 
  8     @Override 
  9     public void setMapValue(Container container) {
 10         // Do something like get data from a DB. 
 11         container.setMap1(new HashMap<String, String>());
 12     } 
 13  
 14 }

So what pattern does this conform to? It looks a bit like a State or Strategy pattern. It also looks a bit like a Command pattern. Which would you say it is?