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?

Design Patterns - Chain of Responsibility

Introduction


I am a fan of design patterns.  Patterns provide a common language and understanding of code-bases, and they help us solve problems in useful, understandable and maintainable ways.

Let's take the case I was working on a few months back.  A co-worker and I were given a project to produce some reporting.  The data being reported on was coming from 3 different data sources and needed to be normalized and scrubbed.  A decision was made to create a data-mart, where the data from the real-time production systems would be copied into the data mart weekly to allow managers to run large reports.

From discussions with managers during the discovery phase one thing became clear : There was general disagreement on how to scrub the data. This data deals with time reporting, and sometimes records didn't necessarily make sense - For instance, workers reported 'starting' and 'ending' work 1 week apart.  Clearly, they didn't work an entire week without rest, or food.  How should this record, then, be handled?  Should the number be truncated? Should we assume the worker meant the SAME day?  Should the record be thrown out? I know some of you out there are saying "Um, contact the worker and get him to fix it.".  In this case, that is not an option, and would likely be a post for a different site.  I think you can imagine that managers could think of an infinite number of ways to 'transform' the data before placing it into the data mart.

The Implementation


It is probably clear from the description about that a routine that using things like if/then's is going to be an error prone, and difficult system to maintain.  First off, when the first change comes in to modify how a record is handled, the developer will first have to find the correct if statement in a large chain of if's.  Second, the concerns are mixed together.  In addition, if a NEW type of data error is discovered that will require a new if statement.  Also, if a new variant on an existing error is discovered, a new if statement might be accidentally created if the developer making the change doesn't realize that the new requirement is simple a variant on an existing error.  Finally, given the amount of disagreement between the managers, it was clear that were going to go back and forth on what transformations they were going to want.  With the if/then implementation, this means a second level of if statements to determine if the transformation is needed, further obfuscating the intent of the code.

Design patterns to the rescue.  After a short discussion, it was clear that we wanted to use was the Chain of Responsibility (CoR).  The CoR describes a pattern that defines a series of processing objects and the operations they can perform, along with a way to add new processing objects on the end of the chain.

Let me explain how the solution was constructed.  Of course, I am not using the actual implemenation from work, but a variation on it in order to demonstrate the pattern.  First, we created an object that can needs to be operated on, in our case, I'll call it a Time object.  Then we created an abstract class called Transform that Takes a time object in its perform() method and a Transform object in its constructor. Then we created Transform implementations for the logical transformations needed.  Here are a couple of things the Implementations may have had:

1. MultipleDayEntryTransformImpl
2. ZeroTimeTransformImpl
3. StartTimeAfterEndTimeImpl
4. ExactlyFiveMinutesImpl

Here is some sample Java code:
/*Here is the class on which the Transform classes will operate.
*/
class Time {
   private Date start;
   private Date end;
   
   public Date getStart(){ return start};
   public Date getEnd() { return end};
   public void setStart(Date start){this.start = start };
   public void setEnd(Date end) {this.end = end};
}


/*Here is the abstract class that defines the interface and constructor
*/
abstract class Transform {
   /*
   */
   public Transform nextTransformation;
   public Transform(Transform nextTransformation){
      this.nextTransformation = nextTransformation;
   };


  public abstract perform(Time time);
}


/*Here is a simple, sample implementation
*/
class ZeroTimeTransformImpl {
  public perform(Time time) {
    if(time.getStart().equals(time.getEnd())) {
      //Do something because the record meets the criteria for this error
    }
    
    if(nextTransformation != null) {
       nextTransformation.transform(time);
    }
  }
}


/*Here is a partial class that might use these...
*/


class Importer {
  public static final main(String[] args) {
    //create the transformers.  this could be defined in
    //a DB, properties file, or made selectable through a GUI..
    ZeroTimeTransformImpl zeroTime = new ZeroTimeTransformImpl(new ExactlyFiveMinutesImpl(...any number of transform objects here));
    
     
    List times;
    //get Time objects, not implemented
    for(Time time : times) {
      //this will perform all the transformations in sequence
      zeroTime.perform(time); 
    }
    //store the normalized time records...
  }
}

As you can see from the code, the pattern has done a few things for us that are desirable when maintaining code:

• The types of transformations are declarative and clear
• The logic to determine if a transformation is needed, and what that transformation is are segregated from all other code and put in their own little world, making it very clear to the developer what is going on
• Many new transformations can be added to the system without having to modify the main codebase at all.  A new instance and a change to the zeroTime instantiation, and you are done (The creation of the Transform objects can also be abstracted, but that's a different pattern.)
• Business users can think up any number of transformations for the Time records, and the system should be able to handle it relatively easily.
• The work of implementing the the transformations can easily be distributed among multiple developers without worrying about merges.

Best of all, I can tell my peers that we used a CoR pattern to solve this problem, and they will immediately have an idea on how the problem was solved, how the code is structured, and where to look then attempting to make changes.