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”.
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.