A Tale of Code Review…

A few years ago, I was working on a project where our client asked for code review by experts from another company. The idea behind this was to assess that we were doing a good job, fair enough. Since they didn’t have any in-house experts on the matter, the only choice left was to hire experts on the topic to review our work.

I don’t remember all the details of this code review, but I have two remarks that distinctly remember because they shocked me.

No Need to Have a Static Private Object to Lock On, Just Lock the Type

Now when I was told that, I was hit very hard because I was certain that the language specifications specifically mentioned that as a bad practice. Turns out that I remembered well: http://msdn.microsoft.com/en-us/library/c5kehkcz.

The good thing is that this one, I managed to avoid having to change it in the code. I don’t remember very well the why, but it was a small victory.

Don’t Use the “+” Operator on String, Use String.Concat

This one was also a good one. When I was told that I lost all hopes that these experts would be useful, as this comment revealed their ignorance of the C# compiler behavior. Not that I think everyone should know that, but I don’t consider myself as a C# guru, but I believe that any enthusiast that played with reflector a little bit should have noticed that.

Not only the “+” operator is replaced by String.Concat call if the content of both operand is not known at compile time, but if both are known at compile time then it is simply replaced by one single String! So, applying this guideline did reduce performances… Not to a noticeable way, I agree, but having to go in your code and replace good stuff with something not as good is not a pleasant experience.

The “fun” part is that actually we also had the String.Concat comment for the “+=” operator. So, one of my colleague  went through the code and replaced all the

s1 += s2;

by

String.Concat(s1, s2);

which is wrong since the return value is ignored, so original string is not modified. Not only this was a pointless change, but bugs were introduced when doing that mindless task.

Conclusion

I don’t know if there is anything to be concluded from this experience. In my case, the advices provided were mostly useless if not harmful. My personal conclusion is that real experts are a rare breed, and it’s not because someone labeled as an expert and expensive that he is. Even if he comes from a big corporation…

Now there is also a conflict of interest here. When an expert is dispatched on this kind of mission, it would be very difficult to justify his price if he didn’t have anything to comment on. So whatever you have done, the expert will have comments and if quality is good chances are that these comments will be useless. Next time, I’ll put some ducks here and there. Needless to say, if the customer is clueless about the technology, he will take the side of the expert. Which is perfectly understandable, because for that particular manager, not following the expert’s advice can only result in a lot of pain if something happens, even if completely unrelated to the expert’s comments. Another example of C.Y.A.

My third conclusion what that I probably was expecting too much from these experts. To me, the word “expert” means people like those on the top reputation page of StackOverflow.