Do Code Reviews Work?


I've noticed a very disturbing trend that continues to repeat whenever the topic and task of code reviews come up. So much (too much?) attention is paid to the "rules" of combat, everything from the proper etiquette of code reviews to the agreed upon coding standards de jour. Too little attention, to my mind, is paid to the skill set required to actually perform a proper code review. Let me illustrate with an example.

Client 'X' goes through great pains and kerfuffle to define a coding standard. Curly braces go here (hence the name of this blog). Variable are names this way. To comment or not to comment, that is the question, and so on, and so forth.

I submit some code for a pull request. I get back... line noise. Kind of like the automated resume readers that scan for keywords. Rule matching, except done poorly by humans. Did you actually look at the code, or at the Git Diff tool? 

Look at the code? Are you out of your mind? I'm busy, I have real work to do!

I have a class like so:


















I get back some comment about 'SomeOtherBool' and coding standards poo pooing the use of nullable types.

Now, I'm not going to argue about the pros or cons of nullable type, or whether or not they are good. I don't really care. What bothers me is that the reviewer did not bother to look at the class and see that I was matching the existing property already implemented.

I have a rule when touching existing code, especially in a large code base. DO NO HARM. Unless the code is meant to be changed, unless the code is egregiously bad, don't change it. There is no technical or business reason to change 'SomeBool', and no reason not to match it to mirror 'SomeOtherBool' to be the same.

If it's appropriate to make 'SomeOtherBool' a non-nullable type, make a case. I would then argue to make all the fields the same if there is no compelling reason not to, especially if the argument is that its the 'coding standard'.

I dislike rules for rules sake. Rules give us guidance. They prevent 'cowboy coding'. I do like that. Rules should not become a religion.

That's what Agile is for. 😁😋














Comments

Popular posts from this blog

Representing C/C++ unions and bitfields in C#

Implementing the try/catch/finally pattern in C++ using lambdas, or why cant this be as easy as it is in C#?

Dispatch? I'd like to make a call. The difference between Synchronization Context, Task Scheduler and Dispatcher