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.
I would agree with you on that approach for a more complex code. For the example you have given, I would argue that there is nothing you want to test here. Testing the CryptoSecurityProvider should be sufficient enough.
I favor the first approach because is much simpler approach.
I agree with you, that you don’t want to test the StatisticsCodec, but what about the code that uses the StatisticsCodec? Note that I’m talking about a ‘mock’ here, not a ‘stub’, so the verification must happen inside the Encode or Decode method.
I hate these “technical” interfaces. Interfaces were introduced in OOP not to make your code testable. If you need to mock static method, you can use mocking framework which supports it.
You are right, the were not introduced to make your code testable, instead it was extensibility and this was one of the major reasons I chose them. With the approach above, you have a nice and clean extension point and you can easily plugin another codec when needed.
Regarding your statement about mocking: I’m not aware of any popular mocking framework that can handle static members. Rhino.Mocks, Moq, NMock, which one? You would end up creating a wrapper around the static interface.
You could argue that you should use a interface for each class – somewhat like the Header-Files in C++.
It don’t matter that nobody thought on TDD or unit-tests when creating interfaces (BTW: are you so sure on this?) – it’s a rather easy tool to make your code very testable and in C# it’s so incredible easy to do that the addition work is almost non-existing (use something like ReSharper and it’s more or less a few clicks/key-strokes).
mocking static methods on the other hand are hard to do, very unreadable and the popular free frameworks don’t have a good interface for something like this (if any).
And I would argue that you should never mock static (or even private) members as this clearly is against OOP principles and will be hard to read/locate.
Classic-Example: mocking DateTime.Now – if you are *clever* with this, your reader never will find the place where you set up your expected date (was it in the Test.Initialize, some obscure helper, … where?)
You invented a bug while refactoring. The generics are absolutely necessary, since the Json.Convert Method expects generic parameter to know which instance should be created to deserialize the JSON string.
When you replace the generic hardcoded with the base class Models.Statistic you will get instance of this base class (it should maybe be abstract to avoid this kind of mistake..).
So the generic constraints made sure there are only passed Types inherited of Statistic.
Yeah, you’re right. The sample above didn’t take into account that the consumer actually wants to pass instances of derived classes. I wrote this on-the-fly while working in the codebase. If you take a look at the actual commit, you can see that the type parameter is still there. Btw, I think we should add a comment, so that we know what we did here in a few years…