Code Reviews

We often read that code reviews are a good idea.

Bugs can be found, people are encouraged to keep to team standards, know-how is spread in both directions and code becomes more maintainable. Also it helps sharing responsibility.

So basically the time spent for the code reviews is well spent and increases the long term efficiency.

But there are some things to observe concerning code reviews.

I have basically seen two variants. In one case a meeting was held and a certain portion of code was known in advance and discussed in the meeting.

The way I have seen much more often was that someone worked on a branch and before merging that branch into the main branch someone else had to review it. The outcome could be that it was merged into the main branch by the reviewer, that the reviewer gave green light to merge it to the developer or that there were minor finding which could be fixed by the reviewer or the developer prior to immediate merging without further review or there could be larger findings that would require a second review.

In an organization that was still not using git they worked with svn on the main branch and somehow the reviewer had to find out which changes belonged to the review. This was a bit of a pain, but other than that the end results were similar to the process using git.

Some issues need to be observed. I will use antipatterns to clarify them.

In one organization the code reviews were done in meetings. The organization had some kind of „review master“ who participated in these meetings. And he had the power to organize such review meetings whenever he wanted. His understanding of review was to bully people, to show them, how stupid they were and how good he was. So the result was, that such meetings were a nightmare for many developers and they felt bad already during the week before. Then the review meetings were horrible and after that the developer whose code was reviewed was depressed for a week or two. So of course depending on the developer, whose code was reviewed, it was just a slightly unpleasant meeting or it meant serious loss of productivity of one person down to almost zero for a few weeks. No, it did not happen to me, but I saw it happen.

Do not do that. Code review is about improving and learning and not about making people bad or judging about people. Some people even do not like it, if review results are communicated by word where others may hear it. Email or chat or whatever is available is preferred. Better respect that.

Also in a place where meetings were held, someone mentioned in the review that he does not like identifiers with more than 12 characters or so. Now it was a Java program and it is just common practice to have very long identifiers in Java, like XyzCombinerControllerFactory or something like that. It caused a long discussion, which is worth discussing at some point, but in this case it was kind of off-topic. So it is important to find an agreement on general standards on which to base the review. And to respect them during the review, even if you disagree. Of course common sense should always be part of the story.

In another case, there was a change in one layer, which was very important and useful, but in that step it was constrained to one layer. The reviewer was against this change and made the point that he would revert the whole thing, because it does not cover all layers. Since that organization was working with svn, even the change in one layer and the merge afterwards is a big deal, but doing all layers at once would have been unreasonable. Again, there was a lack of agreement on the basis.

Another frequently observed issue is that some reviewers like to make a point of issues like how curly braces are put or how spaces are used or other things that could very well be dealt with by a software automatically. I would like to see a setup, where each developer may choose his formatting convention and on commit everything is automatically converted to a common team standard. So this is not even worth the effort of manual editing, but nobody works like this. But at least we have tools like Sonarqube that can discover issues automatically and it is a waste of time for humans.

Then apart from agreeing that everything is fine, there are different possible outcomes and I think it is important to use them. So I see these possibilities:

Everything is fine
It is OK like that, but I would have done it like that. It is up to you if you change it or not
I would strongly recommend to change it, if you like we can talk about it
This needs to be changed. Please change it and then just merge it. (I trust you!)
This needs to be changed. Please change it and come back for another review after that.

Links

Schreibe einen Kommentar

Antworten abbrechen