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?

Android Application Part 6 - SQLLite Database

This post is part 6 of a series.  Click here to go to part 5.


In my previous post, I walked through the scaffolding for the ListService, which will query a REST Web service and sync data locally.  Of course, the code was just the implementation to allow the service to run in a new Thread.  Today I am going to start implementing the storage part of the application.  Android has a a SQLLite. Database.  The OS makes it available through a service.  It even comes packaged with a SQLLiteOpenHelper class.  The implementation for this one is rather easy.  Here I've modified the sample presented on the Android Developer Site.  First I'll show you the code, then I'll talk through what its doing.   

package com.arciszewski.family.shopping.db;

import android.content.Context;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteOpenHelper;
import android.database.sqlite.SQLiteDatabase.CursorFactory;

public class FamilyListOpenHelper extends SQLiteOpenHelper {

private static final int DATABASE_VERSION = 1;
private static final String FAMILY_LIST_TABLE_NAME = "familyList.db";
private static final String LIST_TABLE_CREATE = "CREATE TABLE LIST (ID INTEGER PRIMARY KEY AUTOINCREMENT, SERVER_ID INTEGER, NAME TEXT, DESCRIPTION TEXT);";
private static final String ITEM_TABLE_CREATE = "CREATE TABLE ITEM (ID INTEGER PRIMARY KEY AUTOINCREMENT, SERVER_ID INTEGER, NAME TEXT, LIST_ID INTEGER NOT NULL, NEEDED BOOLEAN, UPDATED_DATE TEXT, UPDATED_BY TEXT);";

public FamilyListOpenHelper(Context context) {
this(context, null);
}

public FamilyListOpenHelper(Context context, CursorFactory factory) {
super(context, FAMILY_LIST_TABLE_NAME, factory, DATABASE_VERSION);;
}

@Override
public void onCreate(SQLiteDatabase db) {
db.execSQL(LIST_TABLE_CREATE);
db.execSQL(ITEM_TABLE_CREATE);
}

@Override
public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
// TODO Auto-generated method stub

}
}


So what is this code doing? First, I've defined which version of the DB this is:


DATABASE_VERSION = 1;


I like this feature of Android. They have built-in support for upgrading. You'll notice, I have not implemented the onUpgrade method, that is because I have no need for it yet. But when I come out with version 2.0, I will have the opportunity to perform upgrade operations.


Next, I define the table names and create SQL for the List and Item tables.


private static final String LIST_TABLE_CREATE = "CREATE TABLE LIST (ID INTEGER PRIMARY KEY AUTOINCREMENT, SERVER_ID INTEGER, NAME TEXT, DESCRIPTION TEXT);"; private static final String ITEM_TABLE_CREATE = "CREATE TABLE ITEM (ID INTEGER PRIMARY KEY AUTOINCREMENT, SERVER_ID INTEGER, NAME TEXT, LIST_ID INTEGER NOT NULL, NEEDED BOOLEAN, UPDATED_DATE TEXT, UPDATED_BY TEXT);";


Finally, I've implemented a constructor that will always open the Family List Database and return it to the caller. That's it. And that is pretty easy my friends. Of course, I'm not making this available as a service to other applications, which is slightly more complex.


Next, I'll implement some querying and saving.

Android Application Part 5 - Creating the Service

This post is part 5 of a series.  Click here to go to part 4.


In my previous posts, I outlined my general design for this application.  In this post, I will review the creation of the Service class for the Android device. This class will have one purpose: Communicate with the server to sync lists.

One of the things I hadn't decided on was how to construct the Web service.  Do I want to use a SOAP style service or a REST style service? The server side for either of these is pretty well defined for me - I've written both SOAP and REST services.  The unknown is how to build these on the client side.  I did a little Web research, and here are a few of the places I've found that have some good information on these topics:

• It appears that Google isn't interested in including SOAP in the standard Android packages at this time, but there is a 3rd party library that provides this support: kSOAP. 
• Google DOES support JSON with Android packaged,  org.json.*.  I also found a handy RestClient.

Given that JSON is less verbose, and the processing will take fewer resources, I think its a better choice for a constrained device like Android.  I also like that I have a RestClient class that I can use, but I can also see what's going on easily inside it.  So, the transport protocol for my application will be JSON.

Let's talk about the code.  Below is my first attempt at creating the ListService.  It's not fully fleshed out.  This first version is written to put in the Multithreading and get a basic framework for the application.  I will next work on pulling out the data from the JSON objects and store them locally (Using the local HSQL db Android provides).  It's also clear I need to understand Intents and bindings much better before I will be able to complete the app.  Let's start with a discussion on the ListService.  This extends the Android Service class and over rides the onCreate, onStartCommand, and stopService methods.  I also added extra logging to see what's going on.

01 package com.arciszewski.family.shopping.service; 02  03 import android.app.Service; 04 import android.content.Intent; 05 import android.os.IBinder; 06 import android.util.Log; 07  08 public class ListService extends Service { 09   private RunnableListService runnableListService; 10   @Override 11   public IBinder onBind(Intent arg0) { 12      13     return null; 14   } 15  16   @Override 17   public void onCreate() { 18     super.onCreate(); 19     Log.i("BAA", "created"); 20   } 21  22   @Override 23   public int onStartCommand(Intent intent, int flags, int startId) { 24     Log.i("BAA", "starting"); 25     if(runnableListService == null) { 26       runnableListService = new RunnableListService(); 27       new Thread(runnableListService).start(); 28     } 29     return START_STICKY; 30   } 31  32   @Override 33   public boolean stopService(Intent name) { 34     Log.i("BAA", name.toString()); 35     boolean superResult =  super.stopService(name); 36     if(runnableListService != null) { 37       runnableListService.setContinueRunning(false); 38       runnableListService = null; 39     } 40     return superResult; 41   } 42 }

Java2html

The ListService does nothing but create a RunnableListService and start it in a new Thread.  It also kills the flag that keep the polling loop open when stopService is called.  Here is the RunnableService:

01 /** 02  * Class that will poll the server for changes and sleep.   03  * @author barciszewski 04  * 05  */ 06 public class RunnableListService implements Runnable { 07   private static final String SERVER_URI = "http://192.168.1.103/familyList"; 08   private static final String APP_ID = "FL1";  09   private static boolean running = false; 10   private boolean continueRunning = true; 11   @Override 12   public void run() { 13     getListFromServer(); 14   } 15    16   private void getListFromServer() { 17     //don't want to try and connect twice 18     if(!getRunning()) { 19       setRunning(true); 20       RestClient.connect(SERVER_URI); 21       //save list data to local data store... 22        23       setRunning(false); 24     } 25     while(continueRunning) { 26       try { 27         Thread.sleep(1000*60*5); 28       } catch(InterruptedException e) { 29         //do nothing 30       } 31       getListFromServer(); 32     } 33   } 34    35    36   public static synchronized void setRunning(boolean runnnig) { 37     RunnableListService.running = running;  38   }; 39    40   public static synchronized boolean getRunning() { 41     return RunnableListService.running; 42   } 43    44   public void setContinueRunning(boolean continueRunning) { 45     this.continueRunning = continueRunning; 46   } 47    48 }

Java2html

Most of the heavy lifting is not implemented yet, but this provides the structure to poll the server.  I also modified the RestClient slightly to return the JSONObject.

Next - More implementation...