When Code Review Becomes Annoying

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:

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.

One Comment

  1. That is really true, i come to my new company, and then, my colleagues reviews my code, told 30 questions mentioned, all of them are such like: why u use if else not : =, why you set frame not size, why blah blah, i know there are a lot of ways to write a method. but why i should use your way?? and then he declined my request. made me reaaaaaaly mad.

    Reply

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s