How often have you seen code like this?
private void AllocateRoles(RoleChangeInfo roleChangeInfo)
{
foreach (Role role in roleChangeInfo.Roles)
{
if (roleChangeInfo.Person.Age > 21 && roleChangeInfo.Person is Senior)
RoleChangeService.AddRole(rolechangeInfo.Person, role);
}
}
If we deconstruct this code we can see that the method AllocateRoles should received an instance of a RoleChangeInfo which is a container for all the information that is required to add the roles to the appropriate person. If we take a look at the implementation of this type we can see that it’s fairly simplistic. It holds the ID of the Role and a RoleTaken DateTime.
public class Role
{
public int ID { get; set; }
public DateTime? RoleTaken { get; set; }
}
The body of the AllocateRoles method consists of a simple ForEach iteration around the Roles collection and with each role we check to make sure the person is older than 21 and they are of type senior. If both of these conditions are true then we call the RoleChangeService.AddRole method which will perform the work and update the RoleTaken.
I have come across this kind of code many times in the past and it is a good example of where a developer has made a number of assumptions and have not thought about making the AllocateRoles method defensive. So before we jump into making this code defensive let’s take a look at some of the possible issues with this code. Can you see them?
Developer Assumptions
· That roleChangeInfo will not be null
· That the Roles collection will be initialised
· That each role within the Roles collection will not be null
· That the roleChangeInfo.Person will not be null
· That it is ok to call this method when the Person is not a senior
· That it is ok to call this method with when the person is younger than 21
· That it is ok to call this method and no roles are present.
· That it is ok to exit this method without checking to see if our Role.RoleTaken has been set.
· None of the Roles.RoleTaken stamps have already been set.
From the list of items above you get the idea. The code in AllocateRoles takes the path of least resistance and should invalid data be passed in no specific exceptions will be raised. This can result in two possible outcomes. Firstly, the code crashes with a null reference exception and we spend hours trying to find the cause of the issue or the code executes a scenario that is considered invalid. This can lead to data corruption and a large headache trying to figure out where it’s happening.
Enter Defensive coding – When we apply defensive coding techniques to this method we are trying to attain 2 possible outcomes. Firstly we wish to identify all possible errors within our method and throw exceptions if any of them are met. Secondly, we want to ensure that no business functionality will be executed accidently and that the results of our code is sound. Given this, let’s take another stab at our AllocateRoles method.
private void AllocateRoles(RoleChangeInfo roleChangeInfo)
{
//CHECK: that we have a valid rolechangeInfo and that we have a Person and that the person
//is of type Senior and that the roles collection is not null.
if (roleChangeInfo != null && roleChangeInfo.Person != null
&& roleChangeInfo.Person.GetType() == typeof(Senior)
&& roleChangeInfo.Roles != null)
{
// CHECK: that we actuall have roles.
if (roleChangeInfo.Roles.Count == 0)
throw new ApplicationException("You must specify at least one role.");
foreach (var role in roleChangeInfo.Roles)
{
// CHECK: we dont have a null role.
if (role == null)
throw new NullReferenceException("Role cannot be null");
// CHECK: that the Role Taken has not been set.
if (role.RoleTaken != null)
throw new ApplicationException("Role has already been taken.");
// CHECK: that the Persons ages is not less than 21.
if (roleChangeInfo.Person.Age < 21)
throw new ApplicationException("You cannot allocate roles to seniors under 21");
//DO OUR WORK HERE
RoleChangeService.AddRole(rolechangeInfo.Person,role)
}
// CHECK: all roles have a taken date.
if (roleChangeInfo.Roles.Exists(role => role.RoleTaken == null))
throw new ApplicationException("Role taken date has not been set.");
}
}
As you can see the method implementation above now performs a number of checks on the input data and where appropriate throws exceptions if the state is invalid. At this point we could consider this method defensive enough for our purposes. However by implementing numerous checks on the state of the data we have lost readability of our business logic. Secondly the checks are scattered throughout the method and the very first check still falls through and can exit with no code being run. In most cases I may not mind this but others I may want to ensure fall through does not occur. The result is that I would have to break apart this check into multiple If’s. Not ideal.
Enter Arrange, Act, Assert - If you are familiar with unit testing you may have come this pattern when creating your tests. If you haven’t I suggest you have a quick read
here. But the basics of this pattern are that you
arrange your code in a way so that the
Act you are performing can execute with the desired effect and that after the work is done you can
assert any failures. Once you get the hang of it it certainly makes writing tests easier. Given this, I want to refactor the method so that a reusable pattern can be established when performing defensive coding. If we look at our pattern is follows the following workflow.
Assert (Input) >> Act >> Assert (Output)
Given this pattern and I what I want to achieve, I needed to create a simple utility class that would allowing me to perform daisy chaining of conditions which can then be executed at a given point. The Defensive methods such as IsNull would return instances of themselves allowing semantics such as the following.
Defensive.CurrentMethod
.IsNotNull(test, "Message")
.IsInstanceOfType(new object(), typeof(object), "Type does not match")
.IsTrue(() => 1 == 2, "1 wont equal 2")
.Check();
Using this new utility class we can now go back to our AllocateRoles method and refactor to meet AAA pattern we use in our tests. Below is the final defensive method.
private void AllocateRoles(RoleChangeInfo roleChangeInfo)
{
//ASSERT Inputs
Defensive.CurrentMethod
.IsNotNull(roleChangeInfo, "Cannot call AllocateRoles without a roleChangeInfo")
.IsNotNull(roleChangeInfo.Person, "You must have a valid Person reference")
.AnyNull(roleChangeInfo.Roles, "You must specify roles")
.IsInstanceOfType(roleChangeInfo.Person, typeof(Senior), "You can only apply roles to senior people")
.IsTrue(() => roleChangeInfo.Person.Age > 21, "You cannot allocate roles to seniors under 21")
.IsTrue(() => roleChangeInfo.Roles.Count > 0, "You must specify at least one role.")
.Check();
//ACT
roleChangeInfo.Roles.ForEach(role => RoleChangeService.AddRole(rolechangeInfo.Person, role));
//ASSERT Outputs
Defensive.CurrentMethod
.IsTrue(() => !roleChangeInfo.Roles.Exists(role => role.RoleTaken == null), "Role allocation failed.")
.Check();
}
You can see that the AAA syntax coupled with the daisy chaining allows for our defensive checks to be grouped together at the top and bottom of our method. This avoids diluting our business code with numerous checks. Defensive coding is a powerful tool in our arsenal when it comes to increasing the resilence of our application code.