Who hasn't been into this position? I think we all know the drill: "we'd like you to do it know and you have until xxx to fix it". How do you approach such a situation? Do you open the source and start hammering code in an unbelievable pace or do you think the changes through, think where you will be invasive and what tests need to be written?
This (long overdue) first post is regarding making those changes. I will assume you have a working understanding about this.
Right, let's get started. If you don't have much time and you still need to make the change there are some techniques that let you safely insert / inject code while minimizing the risk of introducing errors.
Sprout Method
WEWLC: "When you need to add a feature to a system and it can be formulated completely as new code, write the code in a new method. Call it from the places where the new functionality needs to be"
This is a nice trick in that it will allow you to write the new code in a test harness provided you can instantiate the system under test (SUT).
Suppose we a class TransactionGate that is used to post entries. These entries need to have a certain timestamp that is applied before adding the entries to the transactionBundle.
public class TransactionGate
{
readonly IList<Entry> transactionBundle = new List<Entry>();
public void PostEntries(List<Entry> entries)
{
entries.ForEach(entry =>
{
entry.PostDate();
transactionBundle.Add(entry);
});
}
}
Now the requirement is that we check whether an entry is already listed in the transactionBundle before we add it. Fair enough, we change the code to look like this:
public class TransactionGate
{
readonly IList<Entry> transactionBundle = new List<Entry>();
public void PostEntries(List<Entry> entries)
{
entries.ForEach(entry =>
{
entry.PostDate();
if (!transactionBundle.Contains(entry))
{
transactionBundle.Add(entry);
}
});
}
}
Contrived as it may look like we are actually making the code worse. What if the next day another requirement is to alter the conditional? It would be very tempting to simply add the next bit of code as well making the code start to rot.
A better solution is to use Sprout method. In this case, filter out the entries that are already in the transactionBundle and only add those.
public class TransactionGate
{
public TransactionGate()
{
TransactionBundle = new List<Entry>();
}
public IList<Entry> TransactionBundle { get; set; }
public void PostEntries(IList<Entry> entries)
{
var newEntries = FilterEntries(entries);
newEntries.ForEach(entry =>
{
entry.PostDate();
TransactionBundle.Add(entry);
});
}
IList<Entry> FilterEntries(IEnumerable<Entry> entries)
{
return entries
.Where(s => !TransactionBundle.Contains(s))
.ToList();
}
}
It would be trivial simple to unit test the newly created method:
public class TransactionGateTests: ContextSpecification<TransactionGate>
{
[Test]
public void when_an_entry_is_added_that_is_already_in_the_transactionBundle_it_should_not_Be_added()
{
var transactionGate = new TransactionGate();
transactionGate.PostEntries(new[] {new Entry(1), new Entry(2)});
transactionGate.PostEntries(new[]{new Entry(1), });
transactionGate.TransactionBundle.Count().ShouldEqual(2);
}
}
------ Test started: Assembly: Koning.WEWLC.Tests.dll ------
1 passed, 0 failed, 0 skipped, took 1.23 seconds.
However, if you then create this test:
[Test]
public void when_duplicate_entries_are_added_that_were_not_before_in_the_bundle_only_unique_entries_should_be_added()
{
var transactionGate = new TransactionGate();
transactionGate.PostEntries(new[] { new Entry(1), new Entry(1) });
transactionGate.TransactionBundle.Count().ShouldEqual(1);
}
if fails miserably:
------ Test started: Assembly: Koning.WEWLC.Tests.dll ------
TestCase 'Koning.WEWLC.Tests.TransactionGateTests
.when_duplicate_entries_are_added_that_were_not_before_in_the_bundle_only_unique_entries_should_be_added' failed:
Expected: 1
But was: 2
at Machine.Specifications.NUnitShouldExtensionMethods.ShouldEqual(Object actual, Object expected)
C:\Dev\Personal\WEWLC\Koning.WEWLC\Koning.WEWLC.Tests\Tests\TransactionGateTests.cs(25,0): at Koning.WEWLC.Tests.TransactionGateTests.
when_duplicate_entries_are_added_that_were_not_before_in_the_bundle_only_unique_entries_should_be_added()
1 passed, 1 failed, 0 skipped, took 1.24 seconds.
If you take a look at the initial code we wrote then it becomes obviously clear that there is a huge benefit to writing the code in a new method: the code to PostEntries does not need to change, we only need to correct the code in the GetNewEntriesFrom method.
We now have a test that fails, good! Let's add a Distinct() clause to the filtering code so that the test passes:
public class TransactionGate
{
public TransactionGate()
{
TransactionBundle = new List<Entry>();
}
public IList<Entry> TransactionBundle { get; set; }
public void PostEntries(IList<Entry> entries)
{
var newEntries = FilterEntries(entries);
newEntries.ForEach(entry =>
{
entry.PostDate();
TransactionBundle.Add(entry);
});
}
IList<Entry> FilterEntries(IEnumerable<Entry> entries)
{
return entries
.Distinct()
.Where(s => !TransactionBundle.Contains(s))
.ToList();
}
}
Only a subtle change but one that would not have been obvious if it were not for the tests written.
Sprout Class
The mechanics between Sprout Class and Sprout Method are not that different. You use Sprout class if the logic you want to add really belongs in its own separate class. You can also use Sprout Class if the class to alter is difficult to get into a test harness.
Wrap Method
When you encounter a method that consists a lot of procedural code it is often easiest to first wrap the existing method into another method. Say we have the Following PayDispatcher class:
public class PayDispatcher
{
public void Pay(Employee employee)
{
//.... a lot of code follows
}
}
public class Employee
{
}
Suppose we are required to add logging to the Pay method following. With Resharper this extraction is made extremely easy using the key combination CTRL-R, M (Extract method). We first move the code in Pay to DispatchPayment() and then add logging.
You will probably end up with something like this:
public class PayDispatcher
{
public void Pay(Employee employee)
{
DispatchPayment(employee);
LogPayment(employee);
}
void DispatchPayment(Employee employee)
{
//.... a lot of code follows
}
void LogPayment(Employee employee)
{
//.... some logging code follows
}
}
Now, the point here is to see how wrapping the method calls would increase readability and isolation of the changes. Obviously the logging could be done in several other ways.
Wrap Class
One of the (many) alternatives would be to completely wrap the logging into a decorator. Let's have a look.
We first have to undo the logging in the original PayDispatcher class:
public class PayDispatcher
{
public void Pay(Employee employee)
{
DispatchPayment(employee);
}
void DispatchPayment(Employee employee)
{
//.... a lot of code follows
}
}
We can then write a decorator PayDispatcher that will wrap our existing PayDispatcher and apply the required logging, like so:
public class LoggingPayDispatcher
{
readonly PayDispatcher payDispatcher;
public LoggingPayDispatcher(PayDispatcher payDispatcher)
{
this.payDispatcher = payDispatcher;
}
public void Pay(Employee employee)
{
payDispatcher.Pay(employee);
LogPayment(employee);
}
void LogPayment(Employee employee)
{
//.... some logging code follows
}
}
This concludes the various approaches found in WEWLC for "I don't have much time and I have to change it". Hope you liked it, let me know where to improve.