Refactoring to Strategy. My example. Part 1

Posted by erka on January 21st, 2007 under development refactoring

I am writing about refactoring again. I found a class that has a code which is not so clean as I want. I researched it and understood that I could make it better. It’s a good chance to demonstrate how to refactor code using pattern Strategy. Let’s go.

We have following code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
public class ConnectionManager {
 private static Logger logger = Logger.getLogger(ConnectionManager.class);
 public static final String CONTEXT_PROPERTIES_DEFAULT = "context";
 public static final String COMMON_PROPERTIES_DEFAULT = "common";
 private static final String DS_JNDI_NAME = "connection.jndi_name";
 private DataSource dataSource = null;
 private Context context = null;
 private boolean isDataSourceUsed = false;
 private boolean isPropertyInit = false;
 private String url = null;
 private String password = null;
 private String login = null;
 private String commonProperties;
 private String contextProperties;
 private boolean autoChoose;
 private static final String CONNECTION_AUTO_CHOOSE = "connection.auto_choose";
 private static final String CONNECTION_USE_DATASOURCE = "connection.use_datasource";
 private static final String CONNECTION_DRIVER_CLASS_NAME = "connection.driver_class_name";
 private static final String CONNECTION_DATABASE_URL = "connection.database_url";
 private static final String CONNECTION_DATABASE_USER = "connection.database_user";
 private static final String CONNECTION_DATABASE_PASSWORD = "connection.database_password";

 public ConnectionManager() {
   this(COMMON_PROPERTIES_DEFAULT, CONTEXT_PROPERTIES_DEFAULT);
 }

 public ConnectionManager(String commonProperties, String contextProperties) {
   this.commonProperties = commonProperties;
   this.contextProperties = contextProperties;
 }

 private DataSource createDataSource() throws NamingException {
    ResourceBundle contextRes = ResourceBundle.getBundle(commonProperties);
    String strJNDI = contextRes.getString(DS_JNDI_NAME);
    if (context == null) {
       context = getRootContext();
    }
    dataSource = (DataSource)context.lookup(strJNDI);
    return dataSource;
 }
 private Context getRootContext() throws NamingException {
    // create property for initial context
    Properties properties = new Properties();
    //read property from resurce bundle
    ResourceBundle contextRes = ResourceBundle.getBundle(contextProperties);
    Enumeration keys = contextRes.getKeys();
    if (keys != null) {
       while(keys.hasMoreElements()) {
         String key = (String)keys.nextElement();
         String value = contextRes.getString(key);
          // add property
         properties.put(key, value);
       }
    }
    context = new InitialContext(properties);
    return context;
 }
 private DataSource getDataSource() throws NamingException {
    if (dataSource == null) {
        createDataSource();
    }
    return dataSource;
 }
 public Connection getConnection() throws Exception {
    Connection conn = null;
    try {
      if (!isPropertyInit) {
         ResourceBundle contextRes = ResourceBundle.getBundle(commonProperties);
         autoChoose = new Boolean(contextRes.getString(CONNECTION_AUTO_CHOOSE)).booleanValue();
         isDataSourceUsed = new Boolean(contextRes.getString(CONNECTION_USE_DATASOURCE)).booleanValue();
         try {
            Class.forName(contextRes.getString(CONNECTION_DRIVER_CLASS_NAME));
            url = contextRes.getString(CONNECTION_DATABASE_URL);
            login = contextRes.getString(CONNECTION_DATABASE_USER);
            password = contextRes.getString(CONNECTION_DATABASE_PASSWORD);
         } catch (Exception ex) {
            logger.error("exception while getting datasource", ex);
         }
      }
      isPropertyInit = true;
      if (isDataSourceUsed) {
         try {
           conn = getDataSource().getConnection();
         } catch (Exception ex) {
           logger.error("while getting datasource", ex);
         }
         if (autoChoose && conn == null) {
           logger.info("Can't find a datasource. Using direct connection");
           conn = getConnectionFromURL();
         }
      } else {
         conn = getConnectionFromURL();
      }
    } catch (Exception ex) {
      logger.error(ex, ex);
      throw ex;
    }
   return conn;
 }
 private Connection getConnectionFromURL() throws SQLException {
   Connection conn = DriverManager.getConnection(url, login, password);
   return conn;
  }
}

Where are the problems? First of all, method getConnection has to many conditions and contains initialization logic. In this article I will prepare a code for extracting different strategies.

Step 1. Remove duplications.The first duplication is:

1
ResourceBundle contextRes = ResourceBundle.getBundle(commonProperties);

File with properties has been loaded twice (getConnection, createDataSource), and contextRes variable has been used for different purposes. It’s wrong. Variable for commonProperties should be renamed to commonRes. I will move resources to class fields and load them in the constructor only once. So,

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
public class ConnectionManager{
 //....
 private  ResourceBundle commonRes;
 private  ResourceBundle contextRes;
 public ConnectionManager() {
  this(COMMON_PROPERTIES_DEFAULT, CONTEXT_PROPERTIES_DEFAULT);
 }
 public ConnectionManager(String commonProperties, String contextProperties) {
   this.commonProperties = commonProperties;
   this.contextProperties = contextProperties;
   this.commonRes = ResourceBundle.getBundle(commonProperties);
   this.contextRes = ResourceBundle.getBundle(contextProperties);
 }
 //...
}

Step 2. Change signature of createDataSource. Returned value is not using anywhere. I will change method’s name:

1
2
3
4
5
6
7
private void initDataSource() throws NamingException {
   String strJNDI = commonRes.getString(DS_JNDI_NAME);
   if (context == null) {
     context = getRootContext();
   }
   dataSource = (DataSource) context.lookup(strJNDI);
 }

Step 3. Let’s review method getConnection. I think it’s a good idea to use ‘extract method’ refactoring template in this case:

1
2
3
4
5
6
7
8
try {
   Class.forName(commonRes.getString(CONNECTION_DRIVER_CLASS_NAME));
   url = commonRes.getString(CONNECTION_DATABASE_URL);
   login = commonRes.getString(CONNECTION_DATABASE_USER);
   password = commonRes.getString(CONNECTION_DATABASE_PASSWORD);
 } catch (Exception ex) {
   logger.error("exception while getting datasource", ex);
 }

initDirectConnectionParams will be a good name for this method.

Step 4. Variable isPropertyInit is redundancy and I want to remove it. Following lines will be moved to constructor:

1
2
3
autoChoose = new Boolean(commonRes.getString(CONNECTION_AUTO_CHOOSE)).booleanValue();
isDataSourceUsed = new Boolean(commonRes.getString(CONNECTION_USE_DATASOURCE)).booleanValue();
initDirectConnectionParams();

Step 5. I’m using extract method again for getDataSource().getConnection(). New method will be getDataSourceConnection.

Step 6. I want to remove getDataSource method. I will move initDataSource() to the constructor and replace getDataSource with Inline Method.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
public ConnectionManager(String commonProperties, String contextProperties) {
   this.commonProperties = commonProperties;
   this.contextProperties = contextProperties;
   this.commonRes = ResourceBundle.getBundle(commonProperties);
   this.contextRes = ResourceBundle.getBundle(contextProperties);
   autoChoose = new Boolean(commonRes.getString(CONNECTION_AUTO_CHOOSE)).booleanValue();
   isDataSourceUsed = new Boolean(commonRes.getString(CONNECTION_USE_DATASOURCE)).booleanValue();
   initDirectConnectionParams();
   if (isDataSourceUsed){
      try {
         initDataSource();
      } catch (NamingException e) {
         throw new RuntimeException(e);
      }
   }
 }
 private Connection getDataSourceConnection() throws SQLException, NamingException{
    return dataSource.getConnection();
 }

So, what do we have now?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
public class ConnectionManager {
    private static Logger logger = Logger.getLogger(ConnectionManager.class);
    public static final String CONTEXT_PROPERTIES_DEFAULT = "context";
    public static final String COMMON_PROPERTIES_DEFAULT = "common";

    private static final String DS_JNDI_NAME = "connection.jndi_name";

    private DataSource dataSource = null;
    private Context context = null;

    private boolean isDataSourceUsed = false;
    private String url = null;
    private String password = null;
    private String login = null;
    private String commonProperties;
    private String contextProperties;
    private boolean autoChoose;
    private static final String CONNECTION_AUTO_CHOOSE = "connection.auto_choose";
    private static final String CONNECTION_USE_DATASOURCE = "connection.use_datasource";
    private static final String CONNECTION_DRIVER_CLASS_NAME = "connection.driver_class_name";
    private static final String CONNECTION_DATABASE_URL = "connection.database_url";
    private static final String CONNECTION_DATABASE_USER = "connection.database_user";
    private static final String CONNECTION_DATABASE_PASSWORD = "connection.database_password";
    private ResourceBundle commonRes;
    private ResourceBundle contextRes;

    public ConnectionManager() {
        this(COMMON_PROPERTIES_DEFAULT, CONTEXT_PROPERTIES_DEFAULT);
    }

    public ConnectionManager(String commonProperties, String contextProperties) {
        this.commonProperties = commonProperties;
        this.contextProperties = contextProperties;
        this.commonRes = ResourceBundle.getBundle(commonProperties);
        this.contextRes = ResourceBundle.getBundle(contextProperties);
        autoChoose = new Boolean(commonRes.getString(CONNECTION_AUTO_CHOOSE)).booleanValue();
        isDataSourceUsed = new Boolean(commonRes.getString(CONNECTION_USE_DATASOURCE)).booleanValue();
        initDirectConnectionParams();
        if (isDataSourceUsed) {
            try {
                initDataSource();
            } catch (NamingException e) {
                throw new RuntimeException(e);
            }
        }
    }

    private void initDataSource() throws NamingException {

        // get JNDI name from properies
        String strJNDI = commonRes.getString(DS_JNDI_NAME);

        if (context == null) {
            context = getRootContext();
        }
        dataSource = (DataSource) context.lookup(strJNDI);
    }


    private Context getRootContext() throws NamingException {
        // create property for initial context
        Properties properties = new Properties();

        Enumeration keys = contextRes.getKeys();
        if (keys != null) {
            while (keys.hasMoreElements()) {

                String key = (String) keys.nextElement();
                String value = contextRes.getString(key);
                // add property
                properties.put(key, value);
            }
        }
        context = new InitialContext(properties);

        return context;
    }

    public Connection getConnection() throws Exception {
        Connection conn = null;
        try {
            if (isDataSourceUsed) {
                try {
                    conn = getDataSourceConnection();
                } catch (Exception ex) {
                    logger.error("while getting datasource", ex);
                }
                if (autoChoose && conn == null) {
                    logger.info("Can't find a datasource. Using direct connection");
                    conn = getConnectionFromURL();
                }
            } else {
                conn = getConnectionFromURL();
            }

        } catch (Exception ex) {
            logger.error(ex, ex);
            throw ex;
        }
        return conn;
    }

    private Connection getDataSourceConnection() throws SQLException, NamingException {
        return dataSource.getConnection();
    }

    private void initDirectConnectionParams() {
        try {
            Class.forName(commonRes.getString(CONNECTION_DRIVER_CLASS_NAME));
            url = commonRes.getString(CONNECTION_DATABASE_URL);
            login = commonRes.getString(CONNECTION_DATABASE_USER);
            password = commonRes.getString(CONNECTION_DATABASE_PASSWORD);
        } catch (Exception ex) {
            logger.error("exception while getting datasource", ex);
        }
    }

    private Connection getConnectionFromURL() throws SQLException {
        Connection conn = DriverManager.getConnection(url, login, password);
        return conn;
    }
}

This class still has a lot of places for the refactoring. For example,

1
2
autoChoose = new Boolean(commonRes.getString(CONNECTION_AUTO_CHOOSE)).booleanValue();
 isDataSourceUsed = new Boolean(commonRes.getString(CONNECTION_USE_DATASOURCE)).booleanValue();

I would be very happy if you would find such places and post examples in comments. In the next post I planned to continue refactoring this class using Strategy pattern.

Responses

  1. COTOHA wrote:
    2007-01-22 at 4.59 pm

    hmmm…. these code frames makes the post unreadable.
    i vote for a “stretched” design!!!

    this post should be named not “part 1″ but “cleaning up - preparations to real refactoring”.

    so waiting for nicer design as well as for part 2.

  2. COTOHA wrote:
    2007-01-23 at 1.27 pm

    hm. very nice… but why the hell these code frames remain fixed-width?!?

  3. erka wrote:
    2007-01-23 at 9.37 pm

    The css style is cached by your browser :(

  4. COTOHA wrote:
    2007-01-24 at 5.00 pm

    actually you could just not let my previous post go here. this is why you are called moderator here :)

  5. erka wrote:
    2007-01-24 at 5.21 pm

    I have received the same question from other readers. I have decided to write the answer here.

  6. Erka blog » Archive » Refactoring to Strategy. My example. Part 2 wrote:
    2007-01-30 at 8.23 pm

    [...] So… It’s time to finish refactoring the example. In part 1 I have prepared a class for extracting strategies. Let’s look at our code:[...]

Leave a Reply

You can use these tags: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>

© Roman Dmytrenko 2006-2009