Zhengziying's Blog

Overall speaking, code review is useful.

From time to time, my reviewers did catch some bugs that I overlooked, or point out options that fell into my blind spot. Sometimes I learn new things in code reviews. For example, last year a reviewer suggested me use ThreadLocal<T>, which indeed would simplify my code a lot. Some code reviews aren't the code review per se, when it's more about a FYI: "Hi, I have written this code and I like you to be aware of and get familiar with it." That happens when you are the only person in the team truly understand that piece of legacy code. Although such FYI type of code review isn't that much helpful to the author, it's still good to the team.

But sometimes code review can become annoying, especially when people spend time on things that (in my opinion) don't really matter. For examples:

• string.Join() or String.Join()?
• Put using directives inside or outside the namespace block?
• readonly static vs. const
• System.InvalidOperationException vs. just InvalidOperationException
• Optional parameter vs. overloading
• User var or explicit declaration?

I understand there are differences, often very subtle or trivial though, between string vs. String, readonly static vs. const, etc.. But those differences don't do any real harm. Explicit declaration, such as Stopwatch stopwatch = Stopwatch.StartNew(), doesn't make the code harder to read^[1] than var stopwatch = Stopwatch.StartNew(). String.Join doesn't make the code slower than using string.Join. Putting using outside of the namespace block doesn't make the code harder to work on. In addition, by default all versions of Visual Studio put using outside of namespace in the code it generates.

I really don't like to spend time on those things in code reviews. I don't think they matter to product quality and engineers' productivity. There are so much more things that we wish we had more time to spend on to improve quality and productivity. Debating on those things are like debating whether to put one space after a period or two.

What people should do is to make sure that their team has collectively decided on what StyleCop^[2] rules to turn on in their code base and get included in the build. Once that's decided and has taken effect, there will be no debate any more: if the rules are violated, it will be a build error and we don't submit code review until it at least passes build. Simple and clear.

[1] Readability is both objective and subjective. There is no doubt about that a line longer than 120 or 150 characters is hard to read and single letter variable names are hard to read. But whether Stopwatch stopwatch = Stopwatch.StartNew() is harder to read than var stopwatch = Stopwatch.StartNew(), that's really personal.
[2] Or the equivalent of StyleCop in other languages.