Introduction
This is part one of a two part series of articles on Refactoring and Design Patterns.
In this article, we'll take a simple piece of procedural code and see how maintaining it in an unthinking way gradually reduces the readability of code and scatters our rules and logic to different locations.
We'll then see how we can change the code so that the quality improves rather than degrades. Along the way, we'll learn a thing or two about refactoring, inheritance, and Design Patterns.
Background
Refactoring is the process of making a succession of small well-defined changes to code so as to improve it. There are numerous well understood and documented code issues, along with changes that can be made to fix them. These are changes that many programmers make on a day to day basis without even thinking about them. An awareness of refactoring techniques guides us towards writing better code from the start, and puts us on more solid ground whenever we do need to make changes.
Design Patterns are documented solutions to common problems that arise again and again in the design and implementation of software systems. An understanding of Design Patterns will help you produce better software more quickly. Design Patterns also serve as a common language, allowing developers to discuss design alternatives with greater accuracy and shared understanding.
The Design Pattern that we'll be looking at in this article is the Template Method pattern. This Design Pattern is discussed further in the following article, which also looks at an alternative to the pattern: Template Method Design Pattern vs. Functional Programming - by Tuomas Hietanen.
The initial code
Let's start with a simple class called SecurityManager. It provides a method for creating a new user in a database. For the sake of simplicity, I'm not getting into any database code here. I'll just pop a message to the Console in places where database work is supposed to happen.
As you can see, the method does everything that's needed, from encrypting the user's password (in this case, it simply reverses it), to inserting the user details into the database, to creating entries in the Audit trail.
class SecurityManager
{
public void CreateUser(string username, string realName, string password)
{
char[] array = password.ToCharArray();
Array.Reverse(array);
string encryptedPassword = new string(array);
Console.Write(String.Format("Default Behaviour\n
Inserting Details for User ({0}, {1}, {2})\n\n",
username, realName, encryptedPassword));
Console.Write(String.Format("Default Behaviour\n
AuditTrail Create User ({0})\n\n", username));
}
}
Even at first glance, this code has problems. It does too much in one routine. Routines should do one specific thing. In this case, creating a user is clearly a three-step process, but this one routine is doing it all. If we never need to change this code, it wouldn't be the end of the world to leave it as is. On the other hand, if any changes or special conditions need to be implemented, then things are likely to get worse unless we take some action. We'll shortly return to the steps we can take. For now, let's see what happens if we try and add features to the code as it stands.
Our SecurityManager code has been released into production. It's working fine; however, we've been told that it will soon be rolled out to an office in France where the rules for creating a new user are a little bit different.
In France, passwords are not encrypted. They are stored in plain text. This is not good, and they're working on changing it, but our app needs to handle this situation until the changes are made. We'd also like to make it easy to switch on encryption whenever the French office is ready to use it. The simple quick fix is an 'if' statement that only encrypts password when the country is not France. It wouldn't be a huge deal to remove the 'if' statement when the time comes to enable encryption.
We also discover that as we release the code to other countries, there are other special cases. There are three different versions of the database schema, table and column names can vary, and in some cases, we need to insert data into extra tables, so the SQL for creating a new user will be different from country to country. The quick and dirty fix is a 'switch' statement to create the user in the correct way for each version of the schema.
Our revised CreateUser function is shown below, complete with quick fix hacks. Note that COUNTRY and SCHEMA_VERSION are now checked, and they affect how a user is created. In our demo, these are constants, they could come from a config file or from any number of sources.
The important thing to note is that our simple three step routine for creating a user just got a lot more complicated. If we keep going on this path, each time a new variation arises, it could quickly become unmanageable. This isn't good. Complicated routines are exactly where subtle bugs like to hide.
public void CreateUser(string username, string realName, string password)
{
string encryptedPassword = password;
if (COUNTRY != "France")
{
char[] array = password.ToCharArray();
Array.Reverse(array);
encryptedPassword = new string(array);
}
switch(SCHEMA_VERSION)
{
case 2:
Console.Write(String.Format("Schema 2 Behaviour\n
Inserting Details for User ({0}, {1}, {2})\n\n",
username, realName, encryptedPassword));
break;
case 3:
Console.Write(String.Format("Schema 3 Behaviour\n
Inserting Details for User ({0}, {1}, {2})\n\n",
username, realName, encryptedPassword));
break;
default:
Console.Write(String.Format("Default Behaviour\n
Inserting Details for User ({0}, {1}, {2})\n\n",
username, realName, encryptedPassword));
break;
}
Console.Write(String.Format("Default Behaviour\n
AuditTrail Create User ({0})\n\n", username));
}
Every time you are forced to change code, you have an opportunity. You can use a quick fix which gets the job done but degrades the quality of the code, or you can refactor the code that's already there so that the overall quality of the code is actually improved by the change. Let's forget about the changes we need to make, and start by fixing the code that we started with.
Break the algorithm into meaningful steps
We know that the CreateUser method is doing three things, so we should extract each of those things into their own routines.
In the world of refactoring, there's a name for this...Extract Method. There are even detailed instructions on how to do it. Better still, there are a number of Refactoring Tools that will make the changes for you; you just point them at the code that you want to extract, and they do all the work.
Here's how our Security Manager looks now that we've refactored it a little but. Note that our CreateUser method is still a three step process, but now, the three steps are delegated to their own routines.
class SecurityManager
{
public void CreateUser(string username, string realName, string password)
{
string encryptedPassword = GetEncryptedPassword(password);
InsertUserDetails(username, realName, encryptedPassword);
AuditTrailCreateUser(username);
}
private string GetEncryptedPassword(string password)
{
char[] array = password.ToCharArray();
Array.Reverse(array);
return new string(array);
}
private void InsertUserDetails(string username,
string realName, string encryptedPassword)
{
Console.Write(String.Format("Default Behaviour\n
Inserting Details for User ({0}, {1}, {2})\n\n",
username, realName, encryptedPassword));
}
private void AuditTrailCreateUser(string username)
{
Console.Write(String.Format("Default Behaviour\n
AuditTrail Create User ({0})\n\n", username));
}
}
With the code broken into methods that each do one thing, let's see if we can add our special cases to the code as it stands. We could, of course, go into the functions like GetEncryptedPassword or InsertUserDetails and use 'if' statements and 'switch' statements like we did in the original code.
It's marginally better to do this now in smaller routines with one specific task each; however, it still leaves us with logic about special cases scattered throughout our code. I don't like it. In refactoring lingo...It smells.
Another approach is to use inheritance so that we can override specific parts of the algorithm for creating a user.
Extracting a hierarchy
We can use inheritance when we have a class that broadly does what we want, but where we'd like to modify some of its behaviour, perhaps in response to a special case scenario. Sounds perfect.
The only public method of the class is CreateUser. It fairly accurately captures the general algorithm for creating a user. It calls sub routines to encrypt the password, insert the user details, and audit trail the event.
It might be useful to some future programmer to override the CreateUser method and use a completely different algorithm, but for now, let's assume that it works, and that changes will only apply to the sub steps like encrypting passwords.
To allow for inheritance, we need to modify our SecurityManager class to expose those sub methods that we think could be overridden with different behaviour. We don't want people calling these methods directly; to do so would enable someone to insert user records without inserting the necessary AuditTrail records.
We need to change these methods from private to protected. This will let classes that inherit from SecurityManager see the methods, but won't allow other classes in the system call them directly. We also want our sub class to be able to override the behaviour of the three protected methods. To achieve this, we mark the routines as virtual.
We know that the password encryption can vary, and so can the Insertion of User Data. So far, there is no suggestion that the way we update the Audit Trail can vary. We take the decision that the only methods that we will allow to be overridden are EncryptPassword and InsertUserDetails. We leave AuditTrailCreateUser private and non-virtual. AuditTrailing isn't something that programmers should be screwing around with. We don't want someone overriding the method so that no audit trail is kept.
If we hadn't broken the CreateUser method into its three sub routines, we wouldn't have the option to selectively open up part of the algorithm for modification. It would have been all or nothing. Our refactoring is already paying dividends.
What we've actually done is modify our code to match a well known Design Pattern called the Template Method Design Pattern. If you haven't paid much attention to Design Patterns, it's time to start. Virtually, every design problem that you encounter has been solved and documented by someone else. Design Patterns are the language we use to communicate those solutions.
Apart from providing ready made solutions to common problems, Design Patterns can actually reprogram the way you think about software design and programming. Once you grasp a pattern like the Template Method, you will never again write code like the initial code in this article.
Here's our revised SecurityManager code, now containing two protected virtual methods. Apart from these changes, the code is identical to the previous version.
class SecurityManager
{
public void CreateUser(string username, string realName, string password)
{
string encryptedPassword = GetEncryptedPassword(password);
InsertUserDetails(username, realName, encryptedPassword);
AuditTrailCreateUser(username);
}
protected virtual string GetEncryptedPassword(string password)
{
char[] array = password.ToCharArray();
Array.Reverse(array);
return new string(array);
}
protected virtual void InsertUserDetails(string username,
string realName, string encryptedPassword)
{
Console.Write(String.Format("Default Behaviour\n
Inserting Details for User ({0}, {1}, {2})\n\n",
username, realName, encryptedPassword));
}
private void AuditTrailCreateUser(string username)
{
Console.Write(String.Format("Default Behaviour\n
AuditTrail Create User ({0})\n\n", username));
}
}
Creating a subclass
The base class makes inheritance possible, but it's the derived class that actually makes it happen. Let's take the simplest example we can think of. We'd like the process of creating a user to work exactly like it did previously, but we don't want the password to be encrypted at all, this will meet the needs of the French office.
We create a new class called NoEncryptionSecurityManager. It inherits from SecurityManager. We override the GetEncryptedPassword method so that instead of performing encryption, we simply return whatever password is passed in.
class NoEncryptionSecurityManager: SecurityManager
{
protected override string GetEncryptedPassword(string password)
{
return password;
}
}
Note that we don't mention the other methods at all. This is the key to the Template Method Design Pattern. Our new class behaves just like its parent, except for the behaviour that it specifically overrides.
If you want to visually grasp what's happening here, imagine that at runtime, this new version of GetEncryptedPassword is cut and pasted into the base class, replacing the original GetEncryptedPassword method.
We don't always have to completely re-implement the method when we override it. Sometimes we're happy with what it's doing, but we'd like to add an extra step. The next example overrides GetEncryptedPassword, but it calls the original implementation in the base class using the 'base' identifier. It then adds the extra step of converting the result to Upper Case.
class UpperCasePasswordSecurityManager: SecurityManager
{
protected override string GetEncryptedPassword(string password)
{
return base.GetEncryptedPassword(password).ToUpper();
}
}
We're only overriding one method in these examples. The rest of the class works exactly as implemented in the original SecurityManager class. We can override the behaviour of multiple methods just as easily. The following class overrides the GetEncryptedPassword method to do no encryption, and overrides the InsertUserDetails method to perform some additional steps.
class SchemaTwoSecurityManager: SecurityManager
{
protected override string GetEncryptedPassword(string password)
{
return password;
}
protected override void InsertUserDetails(string username,
string realName, string encryptedPassword)
{
base.InsertUserDetails(username, realName, encryptedPassword);
InsertDetailsForABCSystem();
GrantPermissionsForXYZ();
}
private void InsertDetailsForABCSystem()
{
Console.Write("Schema Two Behaviour\n
Inserting Details into ABC System\n\n");
}
private void GrantPermissionsForXYZ()
{
Console.Write("Schema Two Behaviour\n
Grant Permissions For XYZ\n\n");
}
}
We can try out this modified functionality by creating an instance of our new class:
SecurityManager securityManager = new SchemaTwoSecurityManager();
securityManager.CreateUser("daltonr", "Richard Dalton", "GuessThis");
The console output is as follows:
Using subclasses
Instantiating and using subclasses seems to be an area that causes some confusion for those learning about inheritance. The problem seems to revolve around keeping track of which version of a method will be executed.
In our example, we have a base class called SecurityManager. So, we can write some code that uses this class.
static void DoStuffWithSecurityManager(SecurityManager securityManager)
{
securityManager.CreateUser("daltonr",
"Richard Dalton", "GuessThis");
}
OK, a pretty useless routine, but you get the idea. The method accepts a SecurityManager object and calls the only public method that it has, CreateUser. The key thing to note is that any class that inherits from SecurityManager can be passed into this routine, and it will work.
SecurityManager securityManager = new SecurityManager();
DoStuffWithSecurityManager(securityManager);
SecurityManager securityManager = new NoEncryptionSecurityManager();
DoStuffWithSecurityManager(securityManager);
SecurityManager securityManager = new SchemaTwoSecurityManager();
DoStuffWithSecurityManager(securityManager);
With each of the above examples, the resulting behaviour will be different depending on the type of SecurityManager that we pass to the routine. So, the way we instantiate our class is all important. It should be clear that we now have a way to avoid mixing 'if' statements and 'switch' statements into the guts of our code. We can simply move these conditions into one place to ensure that we select the correct type of SecurityManager. Our SecurityManager class and all its subclasses know how to do their various jobs. They don't get bogged down in decisions about special cases.
SecurityManager securityManager;
if (COUNTRY != "France")
{ securityManager = new SecurityManager(); }
else
{ securityManager = new SchemaTwoSecurityManager(); }
DoStuffWithSecurityManager(securityManager);
Console.ReadKey(true);
Even though we can move our conditional code to one place, it can still get very messy and complicated. In part two of this series, we'll go a step further and see how to make things even simpler.
