This is a follow up on this post, where I set out to walk through a bunch of C# code in order to see how we can improve it.
So this was what we had:
Carsten commented that he was worried about SRP, because the methods do the encryption/decryption AND serialization. This concern is absolutely justified and when I looked at the code a second time, I realized that the naming still was wrong and that this caused the SRP problem.
What is a “Codec” supposed to do? Right, it is “encoding” and “decoding”. The fact that it uses JSON serialization and encryption is a detail of the implementation, so renaming the methods to “Encode” and “Decode” should help:
Yes, that looks better.
The next thing I’d like to discuss is whether it was a good choice to us public static methods instead of instance methods. Let us take a look at how this is used:
The problem with this is, that code like this is always hard to test, because you can’t easily fake out the codec. Even worse, it is not extensible at all (imagine you want the consumer choose his own encryption).
The answer is to use instance methods instead of public methods and introduce an interface:
Now we can easily inject the dependency on IStatisticsCodec into the consumer. It really makes no difference if we use an IoC container (Ninject, StructureMap, whatever) or roll our own code for this – most importantly we have created a clean extension point and are able to unit-test this code easily now.
You may be wondering why I even take the time to write this down because it seems so trivial and natural at first – dear friend, please go and take a look at the majority of source code we’re dealing with every day. It’s so easy to get this simple things wrong, no one to blame here.