Right now I’m working on some code not written by me and I need to make some modifications to it. Along the road I’ve come across some issues which I’d like to discuss. Originally I wanted to do this on our internal blog but actually I think there’s no reason why I shouldn’t post it publicly.
So, in this short series I’m going to review some of our own code. It will not be about big things like architecture, platforms, religion or stuff like that. Instead, I’m going to touch on various small things. Sometimes they are so tiny that we don’t even think about them during a ‘normal’ day. Things like how we name methods, format linq queries, use Tuple over Dictionary, etc.
Anyway, if you’re a C# programmer like me, you’re invited to join and discuss my opinions.
I’ll break this up into several parts so that we have small enough portions we can actually talk about. Let’s start with this one:
First thing to notice is the name. Why do we want to call it “StatisticsHelper”? We can actually be much more specific here since it’s all about encrypting and decrypting information. So let’s call it “StatisticCodec”. This makes it clear what it does and probably prevents some F12s.
Next, please take a look at the generic type “T” which is constraint to be Models.Statistic. There’s absolutely no benefit of having it generic, but what it does is that it makes the whole thing more complicated. Let’s remove it.
Then there is the thing with method names. I personally like verbose names for methods but this isn’t right. “Statistic” is already part of the class name and since the purpose is clear, we should rename the methods to “Encrypt” and “Decrypt”.
Last but not and also least, there is the ReSharper suggestion of using the implicitly typed ‘var’. Yes, why not?
Alright, here is how the class looks like after these modifications:
Next time I’ll talk about the implications of using a static implementation and maybe along the lines, we also check what an interface could give us in this case.
As you wanted to discuss – so be it
I fully agree on the naming issues – maybe I don’t really like the “Codec” thing but I just don’t find a better term.
On the generic-removal I’m not too sure.
Yes for now I feel that you should go with YAGNI but I guess the author did have something in mind as he/she made it generic.
On the other had, as a functional programmer I *like* generic code … I’m 80:20 in favor of your change here…
The only other thing I see is that the methods don’t follow the SRP – they do de/encryption AND serialization – but of course refactoring this would yield 1-line methods so …. ???
Depending on the overhead/memory/security issues that might be connected to it I would consider to lift the “provider” into a static class field – but I just don’t know enough to decide.
I agree to the SRP issue and reflect that in the next post. Thanks for pointing that out.
Wouldn’t it be better to somehow make it clear that the string passed to the Decrypt method should be a JSON-string representing a Models.Statistic object and that the Encrypt method returns a JSON-string?
No, I don’t think so. The fact that it uses JSON for serialization should be internal to the codec, so that the consumer of this class doesn’t need to know how the codec works internally.
But shouldn’t the consumer of the class know that the string they need to pass to the Decrypt has be JSON?
Let’s say I’m the consumer of the class and I try to do this: StatisticsCodec.Decrypt(“a new staticstic”); I can’t really know that I’m supposed to pass in a JSON string.
A codec here is a symmetric processing of data, and in general, I as a consumer of this codec, don’t want to know how the codec works internally. Let’s take MP3 for example. If I record some music and encode it with MP3, I don’t want to know how what the MP3 encoding algorithm does. Everything I know is that I need to use an MP3 decoder in order to listen to music. Let’s imagine someday MP3 improves and changes its internal algorithm. As a consumer I don’t really care because I know I’m using an MP3 encoder and decoder, so it is expected to work.
In our example, we could decide to switch from JSON to XML for some reason. In our “decrypt” method we would only need to make sure that we support both formats, so that we can still process old data.
I don’t think you want the information of the used format (JSON or XML) leak trough to the outer layer of the application (the consumer).
I understand and agree. That somewhat implies the JSON-strings have short lives in the application? With short lives I mean they only exist at the very beginning of a chain (eg. you get a string from a file/database/stream… and decode it) or at the very end of a chain (eg. you encode a statistic object right before you save it/stream it) and most of the work is done with an actual statistic object. Am I right in these assumptions?
Yes, kind of. What we actually do here, is that we collect usage statistics and encode them before sending over the wire, so that they are secure even without ssl. So yes, you are right – the encrypted json string representation only lives while being sent/received.