I tweeted something about mental health, medication, and the burden of self-care. It seems worthwhile capturing my thought on here.
Continue reading “Mental Health, Medication and the Burden of Self-Care”
I tweeted something about mental health, medication, and the burden of self-care. It seems worthwhile capturing my thought on here.
Continue reading “Mental Health, Medication and the Burden of Self-Care”
How often do we talk about software ‘ecosystems’ or ‘environments’? Yet how often do we explore the metaphor that underlies these terms? Continue reading “Software Ecology”
I’ve just presented my talk on Unit Testing the Hard Stuff at the European Testing Conference.
I hope to share more material in due course, but for the moment, here are my slides (pdf).
In a code review I encountered some test code that looked a bit like this:
var result = await _controller.Resources() as ViewResult; result.Should().NotBeNull(); // ReSharper disable once PossibleNullReferenceException result.Model.Should.BlahBlahBlah();
This is a typical defensive coding pattern whose reasoning goes like this:
_controller.Resources()
is Task<ActionResult>
.Result
of this Task
to a ViewResult
, as I want to inspect its Model
attribute.Result
could be a different subclass of ActionResult
, so I had better use a safe cast, just in case.Now, defensive coding styles are valuable when we don’t know what data we’ll be handling, but this is most likely to happen at the boundaries of a system, where it interacts with other systems or, even more importantly, humans.
But in the context of a unit test, things are different:
_controller.Resources()
returns any other subclass of ActionResult
, then the fact that it cannot be cast to ViewResult
is the very information I want to receive, as it tells me how my code is defective.This means I can rewrite the code like this:
var result = (ViewResult) await _controller.Resources(); result.Model.Should.BlahBlahBlah();
By setting aside the defensive idiom, I’ve made the test clearer and more precise, without losing any of its value.
We have some NUnit cross-system test assemblies that run in parallel. They upload files and watch to see how they are processed. We were inserting a randomly generated value into the filenames in an attempt to avoid identically named files overwriting each other.
Unfortunately, this didn’t work, and we saw frequent test failures. In particular, we found that filenames generated by one test assembly were often identical to those generated by another.
I wanted to understand why this was happening, so I did some investigation.
We were getting our values fromTestContext.CurrentContext.Random
, which is an instance of NUnit’s Randomizer
class.
When we look at the implementation of Randomizer
, we see this:
static Randomizer() { InitialSeed = new Random().Next(); Randomizers = new Dictionary<MemberInfo, Randomizer>(); }
The Randomizer
is statically seeded with a value generated by the System.Random
class. Because this seed is static, the same sequence of random values is shared by all references to Randomizer
within each assembly, producing a sequence of values that are highly likely to be different from each other. However, references to Randomizer
in a concurrently running assembly use a different instance and have their own sequence of values with its own seed.
Let’s have a quick look at how System.Random
is seeded.
The MSDN documentation tells us:
- The Random() constructor uses the system clock to provide a seed value. This is the most common way of instantiating the random number generator.
And goes on to warn:
…because the clock has finite resolution, using the parameterless constructor to create different Random objects in close succession creates random number generators that produce identical sequences of random numbers.
It would seem that our two parallel test assemblies often start executing within such a short interval of each other, and that NUnit’s Randomizer
is seeded with the same value in each assembly, which means the sequences of values are identical.
There is some discussion of introducing a mechanism for controlling the seeding of Randomizer
in NUnit, but in the mean time, the solution to our problem was to seed our own System.Random
instances, rather than relying on NUnit’s.
I’ve just returned from SoCraTes BE 2017, which took place in the damp and picturesque Ardennes (this leafy picture shows the centre of town!) I come home from every SoCraTes buoyed up my the strength of our community, and full of new ideas and associations.
Here are some of the ideas that excited me this time: Continue reading “Fresh ideas from SoCraTes BE”
In this post I will walk through how to refactor a Factory, moving from a sequence of if
s to a dictionary implementation, and using delegates as a type alias for my object creation methods.
My team have been working on improving the performance our API, and identified a database call as the cause of some problems.
The team suggested three ways to tackle this problem:
Which of these should we attempt first? There was some intense discussion about this, with arguments made in favour of each approach. What we needed was a simple framework for making decisions about how to improve our system.
This is where the Theory of Constraints (ToC) can help. Originally expounded as a paradigm for improving manufacturing systems, ToC is really useful in software engineering, both when managing projects and when improving the performance of the systems we create.
The preliminary step in applying ToC is to identify the Goal of your system. In the case of this API, the Goal is to supply accurate data to consumers.
Now we understand the Goal of the system, we can define the Throughput of the system as the rate at which it can deliver units of that goal, in our case API responses. We can also define the Operating Expenses of the system (the cost of servers) and its Inventory (requests waiting for responses).
The next step is to identify the Constraint of the system. This is the element in the system that dictates the system’s Throughput. In a physical system, a useful heuristic is a build-up of Inventory in front of this element. In our API, our monitoring helped us pinpoint the bottleneck.
The next three steps give us a sequence of approaches for tackling the Constraint:
Exploitation comes first because it’s quick, cheap and local. To Subordinate you need to consider the effects on the rest of the system, but there shouldn’t be significant costs involved. Elevating the Constraint may well cost a fair amount, so it comes last on the list.
Once you have applied these steps you will either find that the Constraint has moved elsewhere (you’ve ‘broken’ the original Constraint), or it has remained in place. In either case, you should repeat the steps as part of a culture of continuous improvement. Eventually you want to see the constraint move outside your system and become a matter of consumer demand.
If we look at the team’s three suggestions, we can see that each corresponds to one of these techniques:
Applying ToC tells us which of these approaches to consider first, namely optimising the query. We can look at caching if an optimised query is still not sufficient, and scaling should be a last resort.
In our case, query optimisation was sufficient. We managed to meet our performance target without introducing additional complexity to the system or incurring further cost.
Goldratt, Eliyahu M.; Jeff Cox. The Goal: A Process of Ongoing Improvement. Great Barrington, MA.: North River Press.
We know that this code violates the Dependency Inversion Principle (DIP):
public class Atm { private readonly InMemoryTransactions _inMemoryTransactions; public Atm() { _inMemoryTransactions = new InMemoryTransactions(new List<Transaction>()); } public void Deposit(int amount) { _inMemoryTransactions.Deposit(amount); } }
In particular, line 3 violates the principle by creating a dependency on a specific implementation, and line 7 violates it by knowing how to construct that dependency.
We can rewrite this code to apply the DIP:
public class Atm { private Transactions _transactions; public Atm(Transactions transactions) { _transactions = transactions; } public void Deposit(int amount) { _transactions.Deposit(amount); } }
Line 3 now depends on an abstraction, and we use constructor injection to supply this dependency.
So how about this code:
public class LoggingInMemoryTransactions : InMemoryTransactions { private readonly Logger _logger; public LoggingInMemoryTransactions(List<Transaction> initialTransactions, Logger logger) : base(initialTransactions) { _logger = logger; } public override void Deposit(int amount) { _logger.Log($"Deposit {amount}"); base.Deposit(amount); } }
Well, this class depends on a Logger abstraction, which is injected in the constructor, so that dependency seems to have been satisfactorily inverted.
However, in line 1 we can see that this class inherits from InMemoryTransactions. This inheritance is also a dependency, and we’re inheriting from a concrete instance, not an abstraction.
If we then look at the constructor on line 5, we can see that it calls the base constructor; in other words, it has to know how its base class is instantiated.
There is a direct parallel between these two cases: each of them has a dependency on a specific concrete class, which is instantiated in a specific way.
We can rewrite this code to use composition rather than inheritance:
public class LoggingInMemoryTransactions : Transactions { private readonly Logger _logger; private readonly Transactions _transactions; public LoggingInMemoryTransactions(Logger logger, Transactions transactions) { _logger = logger; _transactions = transactions; } public void Deposit(int amount) { _logger.Log($"Deposit {amount}"); _transactions.Deposit(amount); } }
This simple example of the Decorator pattern behaves in exactly the same way as the previous implementation, but is now only dependent on abstractions.
If we try to write unit tests around our code, we will soon see the benefit of DIP.
If we try to test our first implementation of Atm
, we find that our test boundary includes InMemoryTransactions
, as that class is a hard-wired dependency. We can’t test the behaviour of Atm
without also testing the behaviour of InMemoryTransactions
. If we also have tests of InMemoryTransactions
, then we may end up duplicated test scenarios.
In this case we have an additional problem: we have only defined read access to the data, so we have no way of testing this class without violating encapsulation:
using NUnit.Framework; [TestFixture] public class AtmShould { [Test] public void Perform_deposit_transaction() { const int amount = 100; var atm = new Atm(); atm.Deposit(amount); // HELP! WHAT CAN I ASSERT AGAINST? } }
In the second implementation, we can write collaboration tests between Atm
and Transactions
, bringing our test boundary in much closer, and restricting the behaviour under test to that of Atm
.
using NSubstitute; using NUnit.Framework; [TestFixture] public class AtmShould { [Test] public void Perform_deposit_transaction() { const int amount = 100; var transactions = Substitute.For<Transactions>(); var atm = new Atm(transactions); atm.Deposit(amount); transactions.Received().Deposit(amount); } }
Similarly, to test the first implementation of LoggingInMemoryTransactions
, we also have to test the behaviour it inherits from InMemoryTransactions
, as one is inseparable from the other. Again, if we also have tests of the base class, then we may end up with duplicated test scenarios.
using NSubstitute; using NUnit.Framework; [TestFixture] public class LoggingInMemoryTransactionsShould { private LoggingInMemoryTransactions _loggingInMemoryTransactions; private Logger _logger; private const int Amount = 100; [SetUp] public void SetUp() { _logger = Substitute.For<Logger>(); _loggingInMemoryTransactions = new LoggingInMemoryTransactions(new List<Transaction>(), _logger); } [Test] public void Log_transaction() { _loggingInMemoryTransactions.Deposit(Amount); _logger.Received().Log($"Deposit {Amount}"); } [Test] public void Store_deposit() { _loggingInMemoryTransactions.Deposit(Amount); // HELP! HOW CAN I CHECK THIS WAS SAVED? // I might be tempted to assert against the new List<Transaction>(), but this would leak implementation details. } }
In the second implementation, the specific behaviour of LoggingInMemoryTransactions
is separated, and we can write collaboration tests independently of the behaviour of the inner implementation.
using NSubstitute; using NUnit.Framework; [TestFixture] public class LoggingInMemoryTransactionsShould { private LoggingInMemoryTransactions _loggingInMemoryTransactions; private Logger _logger; private Transactions _transactions; private const int Amount = 100; [SetUp] public void SetUp() { _logger = Substitute.For<Logger>(); _transactions = Substitute.For<Transactions>(); _loggingInMemoryTransactions = new LoggingInMemoryTransactions(_logger, _transactions); } [Test] public void Log_transaction() { _loggingInMemoryTransactions.Deposit(Amount); _logger.Received().Log($"Deposit {Amount}"); } [Test] public void Store_deposit() { _loggingInMemoryTransactions.Deposit(Amount); _transactions.Received().Deposit(Amount); } }
Updated 26 May 2016: I’ve given code examples for the unit tests we might write in each case, and have removed DateTime createdOn
from the example to avoid confusing the issue.
Updated 26 September 2017: I’ve removed a reference to an individual whose political views are not consistent with the spirit of this blog.
My colleague Pedro and I were puzzling over some bizarre behaviour with ReSharper’s Move Instance Method refactoring move. This is a fairly complex move, and it makes various changes to the source and destination classes; however, in many cases, it generates code that does not compile, and seems to include wrongly qualified references.
Here is an example drawn from Martin Fowler’s Refactoring: Improving the Design of Existing Code:
using System; namespace RefactoringExamples.ReplaceConditionalWithPolymorphism { public class Employee { private EmployeeType _employeeType; private readonly int _monthlySalary; private readonly int _commission; private readonly int _bonus; public Employee(int type) { Type = type; _monthlySalary = 100; _commission = 10; _bonus = 20; } public int Type { get { return _employeeType.TypeCode; } set { _employeeType = EmployeeType.TypeFrom(value); } } public int PayAmount() { return Pay(); } private int Pay() { switch (_employeeType.TypeCode) { case EmployeeType.Engineer: return _monthlySalary; case EmployeeType.Salesperson: return _monthlySalary + _commission; case EmployeeType.Manager: return _monthlySalary + _bonus; default: throw new Exception("Incorrect Employee"); } } } }
using System; namespace RefactoringExamples.ReplaceConditionalWithPolymorphism { public abstract class EmployeeType { public abstract int TypeCode { get; } public static EmployeeType TypeFrom(int value) { switch (value) { case Engineer: return new Engineer(); case Salesperson: return new Salesperson(); case Manager: return new Manager(); default: throw new Exception("Incorrect Employee Code"); } } public const int Engineer = 0; public const int Salesperson = 1; public const int Manager = 2; } class Engineer : EmployeeType { public override int TypeCode => Engineer; } class Salesperson : EmployeeType { public override int TypeCode => Salesperson; } class Manager : EmployeeType { public override int TypeCode => Manager; } }
using System; namespace RefactoringExamples.ReplaceConditionalWithPolymorphism { public class Employee { private EmployeeType _employeeType; private readonly int _monthlySalary; private readonly int _commission; private readonly int _bonus; public Employee(int type) { Type = type; _monthlySalary = 100; _commission = 10; _bonus = 20; } public int Type { get { return _employeeType.TypeCode; } set { _employeeType = EmployeeType.TypeFrom(value); } } public int MonthlySalary { get { return _monthlySalary; } } public int Commission { get { return _commission; } } public int Bonus { get { return _bonus; } } public int PayAmount() { return _employeeType.Pay(this); } } }
using System; namespace RefactoringExamples.ReplaceConditionalWithPolymorphism { public abstract class EmployeeType { public abstract int TypeCode { get; } public static EmployeeType TypeFrom(int value) { switch (value) { case Engineer: return new Engineer(); case Salesperson: return new Salesperson(); case Manager: return new Manager(); default: throw new Exception("Incorrect Employee Code"); } } public const int Engineer = 0; public const int Salesperson = 1; public const int Manager = 2; public int Pay(Employee employee) { switch (System.TypeCode) { case EmployeeType.Engineer: return employee.MonthlySalary; case EmployeeType.Salesperson: return employee.MonthlySalary + employee.Commission; case EmployeeType.Manager: return employee.MonthlySalary + employee.Bonus; default: throw new Exception("Incorrect Employee"); } } } class Engineer : EmployeeType { public override int TypeCode => Engineer; } class Salesperson : EmployeeType { public override int TypeCode => Salesperson; } class Manager : EmployeeType { public override int TypeCode => Manager; } }
Notice the reference to System.TypeCode
in the EmployeeType.Pay
method on line 30 above; this doesn’t compile, and is most definitely not what we wanted. My hypothesis is that once ReSharper has rewritten the code, it attempts to resolve and properly qualify any symbol names; in this case, it spots TypeCode
and, because this is the name of a type known to the compiler, it decides that the name needs to be appropriately qualified, adding the System namespace. It’s failing to recognise that TypeCode
is actually the name of a property on the EmployeeType
class, and that no qualification is necessary.
I have found three ways to work round this:
First, you can give the property a name that not already used by a type known to the compiler. If I rename TypeCode
to Code
, then the refactoring move works perfectly. In this case, this is probably a nice idea, as the name EmployeeType.TypeCode
contains redundancy. However, in many cases the most appropriate name for a property may coincide with a type name, and renaming it will not be a good option.
The second option is to recognise that this happens, and fix it. In this case, all I need to do is remove the System
namespace on line 30, and everything works correctly. I’m not a huge fan of this solution, as I like my automatic refactorings to be safe and reliable, but it may be a pragmatic choice.
The third option is to perform this refactoring by composing smaller, safe steps. This can give you finer grained control over the outcome, but at the expense of complexity.
I would be interested to hear if anyone has any further insight into what’s going on here, or if there are other techniques for overcoming it. Do let me know if you do!
Update (25 May 2016): I sent JetBrains a link to this post, and within an hour they raised a bug report. Thanks to Daria at JetBrains for the speedy response!