Coding tiger, hidden responsibility
During a recent pursuit of the Single Responsibility Principle, I stumbled upon an interesting place a responsibility can hide. Here's our task: we want to write a PersistenceService class that will let us save some Results to a file. These results will be stored as a serialised array of integers (int[]).
I'm going to use a bit of an unusual approach for writing the array out to the file. Commonly we'd have a serialiser class that would open up a file (or ask a dependency to open up the file), write the serialised data, then close and dispose the file. Instead we'll invert control a little. We'll pass in an existing file stream, and the serialiser class will write to it. The type and lifetime of the stream will be managed as an entirely separate responsibility.
Coding tiger
public class PersistenceService {
private IFileStreamer _fileStream;
private IIntegerArrayDataSerialiser _serialiser;
public PersistenceService(IFileStreamer fileStream, IIntegerArrayDataSerialiser serialiser) {
_fileStream = fileStream;
_serialiser = serialiser;
}
public void SaveResults(Results results, string filePath) {
_fileStream.Write(filePath, stream => _serialiser.Serialise(results.GetFoos(), stream));
}
}
Here you'll see we are using an IFileStreamer to manage the lifetime of the stream, and using IIntegerArrayDataSerialiser to manage the serialisation. Our PersistenceService class itself just coordinates those dependencies, which should be its single responsibility. Let's run through the two collaborators before we move on, just so we have a working code sample.
public interface IIntegerArrayDataSerialiser {
void Serialise(int[] data, Stream stream);
}
public class IntegerArrayDataSerialiser : IIntegerArrayDataSerialiser {
private IFormatter formatter;
public IntegerArrayDataSerialiser(IFormatter formatter) {
this.formatter = formatter;
}
public void Serialise(int[] data, Stream stream) {
formatter.Serialize(stream, data);
}
}
We'll create our IntegerArrayDataDeserialiser with a BinaryFormatter (an IFormatter from System.Runtime.Serialization.Formatters.Binary in the framework), and we are basically just wrapping this class. Finally, we have our FileStreamer:
public interface IFileStreamer {
void Write(string path, Action<Stream> streamProcessor);
}
public class FileStreamer : IFileStreamer {
public void Write(string path, Action<Stream> streamProcessor) {
using (var stream = File.OpenWrite(path)) {
streamProcessor(stream);
}
}
}
The FileStreamer has the single responsibility of managing the lifetime of the stream used to write the file. It is basically a creational responsibility. And, believe it or not, this all works just fine.
So, have you spotted the hidden responsibility? It's in the PersistenceService. If you have, then nice work! I didn't spot it at all, although a colleague was able to point it out to me later. What I did spot though was a sign of potential trouble, not from the code, but from the tests...
Testing times
How would you test the PersistenceService as implemented? Despite the simplicity of the implementation, it is quite messy. If you haven't noticed the responsibility have a go at writing a test and see what gives you trouble. Here was how I finally got the test to pass:
public class PersistenceServiceFixture {
[TestFixture]
public class When_saving_data {
private PersistenceService sut;
private FakeFileStreamer fileStreamer;
private FakeSerialiser serialiser;
private int[] data;
private Stream stream;
private string path;
[Test]
public void Should_write_serialised_results_to_file_stream_from_given_file_path() {
Assert.That(fileStreamer.PathUsed, Is.EqualTo(path));
Assert.That(serialiser.DataSerialised, Is.SameAs(data));
Assert.That(serialiser.StreamUsedToSerialise, Is.SameAs(stream));
}
[SetUp]
public void Context() {
stream = new MemoryStream();
fileStreamer = new FakeFileStreamer(stream);
serialiser = new FakeSerialiser();
data = new[] {1, 2, 3, 4, 5};
path = "some path";
sut = new PersistenceService(fileStreamer, serialiser);
sut.SaveResults(new Results(data), path);
}
class FakeFileStreamer : IFileStreamer {
private Stream stream;
public string PathUsed;
public FakeFileStreamer(Stream stream) {
this.stream = stream;
}
public void Write(string path, Action<Stream> streamProcessor) {
PathUsed = path;
streamProcessor(stream);
}
}
class FakeSerialiser : IIntegerArrayDataSerialiser {
public Stream StreamUsedToSerialise;
public int[] DataSerialised;
public void Serialise(int[] data, Stream stream) {
DataSerialised = data;
StreamUsedToSerialise = stream;
}
}
}
}
I had to manually fake out both dependencies. I needed to put some behaviour in my FakeFileStreamer so that it would call our Action<Stream> function that the PersistenceService passes to it from the IIntegerDataSerialiser. By exposing the arguments used to call our FakeFileStreamer and FakeSerialiser we could then assert on them within our test. Not exactly pretty, is it?
What we'd like to do is to use our trusty mocking framework to generate an IFileStreamer and IIntegerArrayDataSerialiser, stub out a value and assert that the stream was written to.
Unfortunately we can't do that, because of a lurking menace...
Hidden responsibility
Let's take a closer look at the PersistenceService.SaveResults() method:
public void SaveResults(Results results, string filePath) {
_fileStream.Write(filePath, stream => _serialiser.Serialise(results.GetFoos(), stream));
}
This is simply coordinating the class' two dependencies, right? It calls the Write() method of our IFileStreamer, and passes in the Serialise() method of our IIntegerArrayDataSerialiser. That coordination is one responsibility. So what is making this so annoying to test? My bearded colleague was able to walk me through the following steps to reveal (and subsequently fix) the problem. (This post is my translation of his explanation. His explanation was great, any mistakes made in this account are mine. :))
Well, firstly, we'd really like to be able to assert something like this in our test:
fileStreamer.AssertWasCalled(x => x.Write(path, stream => serialiser.Serialise(results.GetFoos(), stream));
That way we could be sure that our PersistenceService was producing the expected call to our fileStreamer, passing in the correct pointer to our serialiser.Serialise() method. So what's stopping us? Well the AssertWasCalled() method on Rhino Mocks will check the arguments for the call. It will get the path right, but we are creating a new Action<Stream> delegate to pass in our serialisation function, so we can't compare it to the one used by the PersistenceService.
Did you notice that? I didn't, despite hours of looking :). We are creating a new delegate. If we had that delegate instance we could compare the arguments in our test. But we don't, because of this line in PersistenceService:
public void SaveResults(Results results, string filePath) {
_fileStream.Write(filePath, stream => _serialiser.Serialise(results.GetFoos(), stream));
}
That simple stream => _serialiser.Serialise(...) lambda declaration is creating a delegate instance. So now PersistenceService isn't just coordinating dependencies, but it is creating a new object! That's two responsibilities! We are violating SRP, and that's what our test is telling us.
Resassigning responsibilities
My bearded colleague suggested we move the responsibility for creating this serialisation delegate. Instead of the PersistenceService creating it from our IIntegerArrayDataSerialiser, he reasoned, why not just get the IIntegerArrayDataSerialiser to create it for us? After all, serialisation is its job, and for serialisation our current design needs an Action<Stream>. (Although it is not really an IIntegerArrayDataSerialiser any more, we should probably rename it to a IIntegerArraySerialisationFactory or something. We'll keep it as is just to keep things simple -- its hard enough to follow code samples on a blog without following renames too.)
public interface IIntegerArrayDataSerialiser {
Action<Stream> GetStreamSerialiser(int[] data);
}
public class IntegerArrayDataSerialiser : IIntegerArrayDataSerialiser {
private IFormatter formatter;
public IntegerArrayDataSerialiser(IFormatter formatter) {
this.formatter = formatter;
}
public Action<Stream> GetStreamSerialiser(int[] data) {
return stream => formatter.Serialize(stream, data);
}
}
Notice this class' responsibility is clearer now. Before it was pretty much just a wrapper around IFormatter. Now its responsibility is creational: it is creating a delegate for serialisation from its IFormatter. Let's see how this affects the rest of the code. Our FileStreamer can remain unchanged, so now we just need to update our PersistenceService to call the new factory method on our IIntegerDataArraySerialiser.
public class PersistenceService {
/* ... snip ... */
public void SaveResults(Results results, string filePath) {
_fileStream.Write(filePath, _serialiser.GetStreamSerialiser(results.GetFoos()));
}
}
Now let's see if this makes everything easier to test.
[TestFixture]
public class When_saving_data {
private PersistenceService sut;
private IFileStreamer fileStreamer;
private IIntegerArrayDataSerialiser serialiser;
private int[] data;
private string path;
private Action<Stream> streamProcessor;
[Test]
public void Should_write_serialised_results_to_file_stream_from_given_file_path() {
fileStreamer.AssertWasCalled(x => x.Write(path, streamProcessor));
}
[SetUp]
public void Context() {
streamProcessor = stream => { };
data = new[] { 1, 2, 3, 4, 5 };
path = "some path";
fileStreamer = MockRepository.GenerateStub<IFileStreamer>();
serialiser = MockRepository.GenerateStub<IIntegerArrayDataSerialiser>();
serialiser.Stub(x => x.GetStreamSerialiser(data)).Return(streamProcessor);
sut = new PersistenceService(fileStreamer, serialiser);
sut.SaveResults(new Results(data), path);
}
}
A bit simpler, no? We can now get rid of our hand-coded test doubles and the logic they contained. And now we have access to the Action<Stream> delegate instance that was giving us trouble before. As a result, our assertion (fileStreamer.AssertWasCalled(...)) is very straight forward and is checking that our PersistenceService did exactly what it was meant to.
Now this was mainly done as a coding exercise, so I'm not advocating this design at all, but I do find the realisation that an anonymous delegate or lambda is actually a creational responsibility really useful. It's definitely going to help me divide up responsibilities better in future.
