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

Post a Comment

Popular posts from this blog

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

Image
You are a seasoned C++ applications or embedded programmer, and you need to access an integer bitfield as a set of specific bits. You know how to do this: union   myUnion { unsigned   int   info ; struct { unsigned   int   flag1   :   1 ;   // bit 0 unsigned   int   flag2   :   1 ;   // bit 1 unsigned   int   flag3   :   1 ;   // bit 2 unsigned   int   flag4   :   1 ;   // bit 3 unsigned   int   flag5   :   1 ;   // bit 4 unsigned   int   flag6   :   1 ;   // bit 5 // . // . // . unsigned   int   flag31   :   1 ;   // bit 31 }; }; Supposing, however, you are a C#/.NET programmer. What do you do then? There is no provision or direct support in the language to do this. What tools do you have at your disposal? Well, you do have: [ StructLayout ( LayoutKind . Explicit, Size  =   1 , CharSet  =   CharSet . Ansi)] That will give you the ability to control the byte packing. You also have: [ FieldOffset ( x )]             Where: x
Read more

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

Image
My very first programming 'language' was Z-80 assembler, even before I had learned to program in interpretive BASIC. So you would think I would be used to doing things the hard way, used to pain... =P When I first saw the C language, I fell in love right away. I already had several CPU dialects under my belt, and the idea of pointers was already an easy notion to grasp -- I had already been using VARPTR in BASIC. The idea of a language that was graceful, provided native pointers, language constructs, NO LINE NUMBERS, who could ask for more? Then came C++. Well, not exactly. My first exposure to C++ was via the Glockenspiel "compiler", which was really a glorified C preprocessor. No matter, eventually Borland and Microsoft released C++ compilers, and I never looked back. Until around 2000, when Microsoft released .NET and C#. By this time, we had COM/DCOM, the STL, and boost. Writing custom allocators, IUnknown for custom COM objects, RAII patterns
Read more

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

Image
I recently had to deal with cleaning up  Dispatcher   based UI update code in a WPF application. I am struck after so many years that like garbage collection (think  IDisposable ) and threading, there still does not exist a clear understanding or explanation of how to marshal across threads, the benefits of using constructs like the  SynchronizationContext  to do so, and what problem(s) it solves. I thought it might be instructive to demonstrate the various techniques available to developers using the WPF framework for illustrative purposes. Keep in mind that the concepts outlined here apply not just in WPF, but anywhere where business logic code meets UI presentation. Given a  Dispatcher  object and the  SynchronizationContext  object, which one should you choose, and what are the compelling reasons for doing so? Note: This post was inspired from an answer I posted on  StackOverflow  to this very question. Perhaps it would help to explain what problem the  SynchronizationCo
Read more