What are common code review processes and what is considered bad?

Question

My company recently started doing formalized code reviews. The process goes like this: you submit into a github, request a pull request, code gets reviewed by approximately three people, then if all passes, your code goes in.

The process seems fair, however the three people who do the code reviews don't seem to be fair. I notice that when I put my code in for review, I get anywhere between 100-200 comments. The top number for me was 300 comments once. Of course you'd think it is big changes, but this can be very small changes as well with less than 50 lines of code (which includes unit tests). All comments are considered "must do" and without argument.

With that in mind, my main problem here is that it seems a bit excessive. I talked to the group and they told me basically that just because I had so many years of development in php doesn't mean I am an "developer." Of course this seems more hurtful than not. Also I notice that within the group, they don't appear to produce as much comments and most of the time they ignore or otherwise disregard other comments or suggestions rarely accepting it as a valid point even if something is broken.

So my question is if this is fair? Or common?

Answer 1

All comments are considered "must do" and without argument.

IMHO that's the real issue, since there is no priorization in that. When you get 100-300 comments, there must be some of them having a priority A (real bugs), some of them prio B (likely to lead to bugs later) and some of them prio C (everything else). Tell your colleagues that you are willing to respect all their wishes, but to make changes effective, and your time is limited, insist on a priorization. Then, start with fixing prio A comment first, and if you really have time for more after that, you can start with B (if you are lucky, your boss will understand that fixing prio B and C is not so important, and give you some more important tasks instead of wasting your time).

Answer 2

Code reviews can be a divisive process.

You are at an important junction, though. Do a thoughtful analysis on their review. Are they identifying nit-picking issues, or highlighting serious flaws in your style and logic?

If it's the former, I'd recommend working towards a resolution ( new job, or new code review processes ).

If it's the latter, I recommend doing a lot of code-reading and studying to try to bring your code up to professional quality.

Answer 3

It seems by your comments that your colleagues are using the code review process to agree a methodology or polish the code. I have just started to do code reviews like you and I notice that sometimes we discuss a lot over things that are just implementation approaches or improvements. This is not bad at all as far as its reasonable (300 comments look too many for me, that must look like a reddit thread)

Maybe you need to agree some architectural decisions about the code before start implementing it or maybe is just agree about naming conventions, patterns and good practices so you all know what it is considered "good code".

If you are complying with your code standards as you say and the code works as intended, there shouldn't be so many comments, so either they are using your code as a forum or they are trolling you as it seems that you are pointing.

I would try to be critic with myself, would try to take part in the conversations and see the reason of all this comments and maybe talk with them about it in a constructive way to see why are they so unhappy with your code and if you can code in a way that makes everyone happy and the work don't get stuck in code review.

I just read your last comments, sometimes when you don't agree with the code you can go over it a hundred times and propose changes everywhere that don't make you happy because the real reason its that you would have done a different architectural decision and you just don't like that code, no matter how many times you refactor it. As I say above, maybe you need to agree the approach to the code beforehand so when you write it you know what they expect from it and therefore your code would be more reasonable to them.

Answer 4

From what you are saying, it seems to me that they might have a bias against php developers, and thus they are trying to find every single thing that is wrong with your code in order to prove their point.¹

Regarding the code review itself, I believe as you said already, that such a huge amount of minor comments is less helpful than a few good and valid criticisms. And although I have limited experience with respect to code reviews, the following technique has worked well for the teams I worked-in in the past.

• First of all, a static code analyser should be used to identify most of the issues before the code review takes place. E.g.: running your code through Sonar, Lint or any other good code analyser should help you to get rid of most minor issues. Especially since your reviewers can define custom profiles to ensure everything from bracket placing, white-spaces, comments, proper variable naming and much more...
• Second, I seems to work well if you divide the comments into different categories. For example two categories where one group includes small things that are you should take note of and apply in the future. And a second group for those comments that require an immediate modification of your code which would require another commit and subsequent review. Of course, the number of comments in latter group should be smaller.

Furthermore, I have to say that my first real code reviews also contained more comments than I originally expected. However I never regarded this as a bad thing. If you continue to learn from their comments² and are willing to apply those newly learned techniques / best practices in your future code submissions, the comments should become less. It sure was the case for me ;-)

¹ In my experience, this happens a lot as many programmers claim that php is the most evil programming language, having the most inexperienced programmers using it. I distance myself from this statement as I believe that great software can be written in any language!

² Assuming that although the comments are excessive, some value is in them

Answer 5

Is it common for anyone to get 100+ comments in their code reviews on a routine basis? I would say not. Is it common for people whose code quality "leaves a lot to be desired" to get a lot of comments, absolutely.

However, it also depends on the "rules" of the code review process. EVERYBODY has their own ideas on how something should have been done. If your code review process allows comments to be of the form "You should do it this way instead of that way", then you'll likely get LOTS of comments even for adequate code. If your process is intended to find "defects" then the number of comments should be much smaller.

In my experience, reviews that allow "suggestions" for alternative methods are time wasters. Those "suggestions" should be handled one on one outside the review process. Defect reviews are more useful as they get people to focus on bugs instead of "why didn't you do it like I would have done it?". It also is more useful because there's no denying a bug if someone finds one. Thus, there's no hurt feelings but likely gratitude instead.

UPDATE: With all that said, some code is just plain bad, even if defect free. In that case, the review comment should be a single comment that says something like. "This code needs to be cleaned up. Please postpone the review until the code is discussed with [your name here]." In that case, further review of the code should stop until the comment is rectified.

UPDATE2: @User:Do you discuss your code/design with one of them while you are developing it so you can implement what they are looking for before you get to far doing it your way? Are you changing anything about how you are developing code based on their suggestions or keep thinking your way is fine? Are you learning anything from their comments?

When I am the sw lead on a project, it is my job to be responsible for ALL work products. If I approve a work product then I am claiming the product is acceptable. I want to have a reputation for building quality products. Thus, I have expectations and won't accept less than satisfactory. At the same time I try to teach and explain the reasons for my preferences. Those preferences may not always be ideal (particularly in the eyes of others), but most of those preferences have come from experience. Usually a reaction to avoid repeating the bad ones. Thus, there are a few of my personal "sticklers" that are necessary to get my approval, regardless of pushback.

On the other side, you need to learn the expectations that are necessary to get your work products approved. You can disagree, but since you don't appear to have the authority to over-rule then learn what is expected. I doubt that the team is trying to make you fail. As that makes them look bad also. In that regard, just demonstrate that you are eager to learn (even if you aren't), take what they say and do your best to adapt to their preferences and you'll likely see them back off quite a bit. Maybe find the one that you can at least tolerate and see if they'll do a bit of hand-holding to teach you their ways. Who knows, in the process you may learn something that really could take your skills to the next level.

Answer 6

Some important differences with our team inspection process:

• the basis of an inspection is a checklist, compiled by the whole team.
• Focus is defects (present and future), not style for the sake of style.
• the 3 inspectors (including the author) sit together to run over the comments. Only comments with majority vote remain.

Answer 7

For 50 LOC 300 comments seems to be a bit excessive and - wow - 3 reviewers for every pull request? Your company must have a lot of resources.

From my experience for a useful code review process there must be some rules and/or guidelines:

• Priority of comments
• Classification of comments (Bug, Code Style, etc.)
• Agreed architecture / design to follow
• Agreed code style

If you don't get a priority from the reviewers ask your responsible project manager / team leader; the responsible person for the costs should have an opinion about priorities.

If you have a defined architecture, a common understanding what design patterns you use in your project and an agreed code style, then the review comments should be only about "real issues" like security issues, unintentional bugs, corner cases not covered by the specified architecture, etc.

If your development team has not agreed on "taste issues" (e.g. should a member variable start with "m_"), then every reviewer will force you to follow his style, which is just a waste of time/money.

Answer 8

This really sounds to me like a communication problem. You have an expectation that your code isn't bad enough to deserve 300 comments. The reviewers seem to think you need a lot of feedback. Arguing back and forth in an asynchronous fashion is going to waste a lot of time. Heck, writing 300 comments is a TREMENDOUS waste of time. If these are not all defects then it's possible as a new team member that you don't know the team's style yet. That's normal and something that should be learned to accelerate the whole team.

My suggestion is to save time. Accelerate the feedback. I would:

• Do more interim reviews to avoid making the same mistake and generating the same comment 50 times
• Sit with a reviewer as they review your code so you can talk about the issues as they come up, thus avoiding documenting 300 "issues" that will be wiped away in the next commit
• Pair with one of these "real" developers for some time as you write the code to see what they would do differently

People may argue against pairing because "it will take longer" but that's obviously not an issue here.