Friday, December 22, 2017

Injection: a refactoring exercise.

Yesterday, I needed to extend a program that had a broken design.

The existing program works fine; but it has hard wired into it an identifier for a specific database host.  That host name is used during execution to create the URL string that is passed to JDBC, by merging the hard coded host with a configured database name.

An additional constraint is that I need to have the old and new versions of the program running in parallel, in the same environment.  We're going to run the two reports in parallel for a time, and then decommission the legacy.

So, how to dig my way out?  Here's the outline of how I did it....

As is often the case, I rely on Parnas -- the legacy implementation is the result of a decision that I want to change. Therefore, I begin by creating a boundary around that decision.

In this case, the code in question was computing a new string from the hard coded literal and a member variable.  So I applied an "extract method" refactoring to give me a member function that promised to provide the string that I needed.  The method simply returned the calculated string, as before.

I then created a new type; the configuration model already included DatabaseName as an explicit concept,  but it didn't have an understanding for the URL.  So I introduced a DatabaseUrl type, that was simply an alias for a string.

I then inserted that type into the middle of the method -- so we compute the string, use the string to create a new DatabaseUrl, then use the DatabaseUrl to yield the string to return.

Next, I apply extract method again, creating a function that would convert the DatabaseName to the DatabaseUrl.

Next, I take DatabaseUrl, which had been a local variable, and make it a private member of the object; it now has the same lifetime and scope as DatabaseName, the only different is that it is being initialized at a different point in the program.

Next, I copy the logic to initialize the DatabaseURL to the method where the DatabaseName was initialized.  I now have a redundant copy, which I can remove -- the query that produces the string representation of the url when I need it is just a toString() on the member value

At this point, the only place where the DatabaseName is used in in the initializer, so I can demote that to a local variable in the method, and inline it away.

I don't think we should be using the old method on the API any more, so I mark it as deprecated.

Reviewing at this point -- through a sequence of refactorings, I've managed to extend the existing API, adding the new method I need to it, while at the same time ensure that my existing tests are exercising that code (because the implementation of the old API feeds into the new one).

The legacy implementation includes a factory, which when passed the configuration (captured within a Properties object) returns the interpreted configuration.  So the new piece I need is a factory with the same signature, that understands the new configuration.

But before I can think about creating a new factory implementation, I have to address a second problem -- I was sloppy ensuring the integrity of the composition root.  The factory I need to replace is allocated behind two different default constructors.

So I need to perform a second round  of refactoring.  The member variable for the factory that I need to replace is too specific, as it names the implementation directly.  So the spelling of that type needs to be changed to the interface.  Fortunately, the interface I need is already available, so I can just replace it.

Then we delete the assignment at the member variable declaration.  At this point, everything goes red, because we aren't assigning a final field.  Next, we fix the compilation problem by allocating a new instance of the factory in the constructor body.

Next, we work our way back up the stack to the composition root.  There are a couple possibilities; you can either develop a completely new path from the root to the assignment so that the factory can be passed along, and then with everything wired up remove the duplication, or you can create new signatures, but play criss cross -- the existing implementation gets a new signature, and a new method appears which creates a default factory and then routes to the real implementation. 

I prefer the latter course; you only really need to preserve the signatures that are part of the public API.  Anything that is private to the module you can simply change, watching for local compile errors.

At the end of the game, I have the starting position I'm waiting for: me legacy implementation, with the creation of the default factory at the composition root.

Now I'm ready to make the easy change - implement the new factory, clone the composition root, and replace the legacy factory with the new implementation.