Colleague reviews a same pull request several times instead of once

Question

A senior colleague occasionally reviews some of my pull requests*.

What often happens is: He request changes. I pull them in or reject them after discussing, no problem up to this point. But then it comes: He then grabs another part of the code and reviews that. And then a third time, maybe a fourth time...

That annoys me. All this repetition starts to feel as he is picking on me, even though that is certainly not the case as we get along well, and also that I'm wasting my time looking at each iteration instead of reading it all once.

Note that those are not immense pull requests, rarely something that goes past a hundred line changes. But somehow he always finds something else to review, and he is about the only one who consistently does that.

Although part of the review are nit-picks, his suggestions are constructive and in good-faith as far as I can tell, even when immaterial to the function of the code itself (like rename this variable to some clearer, here you broke the spacing convention etc.). So I am conflicted.

Do I talk to him that his iterative review rounds in a same pull requests is annoying and discouraging or do I put up with it?

*A pull request is a proposed change in existing some existing code. (Mentioning this because this is not a software engineering forum.)

Answer 1

Being a senior developer, almost all the code reviews I perform (and those performed on my own code) are iterative in nature.

Usually, on my first pass I am looking for logical errors, architectural errors, or missed specifications that may require large changes or rewrites. If I find any, I do not continue with the more nit-picky comments (method/variable names, spacing, etc). This is on purpose, because many of those comments may become obsolete after a larger change, so why take the time to review for them?

This means that on subsequent reviews of follow-up commits, I may then add comments on code that has been there since the initial commit. It is not my intention to pick on the developer, but rather to save everyone's time.

In my experience, developers (both junior and senior) will often correct other errors unrelated to a specific comment, if they are within the same block of code, thus lowering the overall quantity of comments.

Of course, this presupposes that all the initial comments are done at once. If they are not, then a conversation with the reviewer to use any available tools to submit comments all together (in a single review) or to ping the developer when the initial review is complete would be constructive and appropriate.

Answer 2

Although part of the review are nit-picks, his suggestions are constructive and in good-faith as far as I can tell, even when immaterial to the function of the code itself (like rename this variable to some clearer, here you broke the spacing convention etc.).

I am going to focus in on this bit for a moment. I work in IT, although not a Dev, I have dabbled with DevOps and am familiar with the process.

There is a world of difference between Code that you only need to maintain for yourself and Code that is needed to be maintained by a team.

When it comes to the latter, things like Readability become major issues.

Spacing, indentation, Bracket useage etc. when these are all consistent across all code, it can make identifying bugs much easier - saving time and business resources.

Conversely, when each Dev does not stick to agreed formats, bugs can easily hide in plain sight all due to a misaligned indent (ask me how I know...)

The more senior you get, the more you will encounter scenarios where this is a factor and the more you will need to ensure that standards are maintained.

Reading between the lines - I think that the problem is that you are trying to do the bare minimum to get onto the next task - and it seems like your Senior is trying to teach you a lesson.

So - my suggestion would be this - next time you get a PR reviewed, instead of just correcting the mistakes noted, take some extra time to go through the whole code, see if there are any areas where things he has raised previously might be applicable - take ownership of your code. Think like a Gordon Ramsay instead of a greasy-spoon line chef.

You aren't just throwing anything out as quick as can to fulfill the customer order, you are instead only sending something out that meets the standards that you uphold.

I have got a virtual $1 that says if you do that, you will find that his reviews become less frequent and less nit-picky, once he sees you taking that extra step to produce your code.

Answer 3

The question to ask yourself is, "do these iterations improve the code, and why couldn't I see those, myself?"

Racing to complete tasks can often introduce new problems.

It seems to me your senior colleague wants a level of "quality" that you don't appreciate.

Today it's whitespace & alignment; tomorrow it's just this once use of "goto" for expediency.

Instead of being annoyed, try to learn from the more experienced mentor.

---

Late addition

Back when my body was more able-bodied, I was fortunate to add some ocean sailing to my CV. The salty-dog captain corrected my speech more than once. Using only wind, waves and currents, true sailors, he said, never sail to a port, but sail toward one.

System develop is, by definition, iterative; shoring-up foundations while advancing.
If there's one bee-line straight to a goal, it's called "typing", not "development".

Answer 4

That annoys me.

That is an absolutely normal reaction. It will not stop to annoy you, even if you get old and wrinkly and "experienced". A Pull Request and Code Review should not be a continuous trickle of mini tasks, constantly interrupting your focus.

Ask your colleague to notify you when their code review is done. And then you fix the issues and ask them to review your fixes. All of them at once.

Answer 5

If he sees something wrong and has a suggestion for how to fix it, a pull request is appropriate along with the problem report.

If he sees something unrelated in the same code, a separate bug report is entirely appropriate.

People don't, and shouldn't be expected to, review every line in full detail before reporting anything. There is nothing wrong with giving, or getting, feedback in stages.

I'm sorry, but as you've described it, you're badly overreacting to the process working as it is intended to work. Learn to embrace incrementality, and to accept that there is always one more bug; peer review is a wonderful tool for finding and fixing them before they go into production.

But also learn when the right answer is "True, but we should open a new issue for that rather than delaying release of this fix." Not all PRs should be accepted into the same change set. If it's better than what was there, and the additional suggestion isn't essential to the issue this change is intended to address, get it merged and then refine further.

If in doubt, ask your team lead for guidance.

Answer 6

Personally, what you describe would annoy me as well.

Since you mention that you have a good relationship with the senior developer, I suggest you go ahead and ask them about this. Focus on your observations and looking for a solution, not how this makes you feel or assigning blame.

In addition, consider your own position: What exactly is annoying you? Returning to the PR again, or more issues being found with your work? Why? Could it just be you? What process would get the code into the same target quality state while avoiding the issues? Merging the changes and making a follow-up PR? Pair programming?

If you have a clear picture of your needs beforehand, it's easier to discuss options and come to an agreeable resolution.

I'd formulate the question to start the discussion like this:

I have noticed that sometimes we go through multiple rounds of reviews on a small(ish) PR. Why do you think this is happening? Is there something we could do to reduce or eliminate this?

There could be a plethora of reasons why the developer is doing this. Maybe they get new ideas each time they see the code with fresh eyes. Maybe they don't have enough time to go through everything at once and try to make at least some progress. Maybe they don't realize. It could be that there is a huge skill gap between you and you both need to adjust. Until you ask, there is no way to know. Understanding the reasons may also help you be less frustrated, even if the process does not change.

The developer could also get defensive. In that case, you probably won't be able to do much about this in short time-frame but could still see improvement if they are able to reflect on this. PR review can also be a power play. The whole point of PRs is gatekeeping.

The entire code review process is culture-specific. Some places do trunk-based development where PRs don't happen at all and reviews are done via pair programming. Other places prefer small PRs and quick resolution with follow-up done in additional PRs unless there are serious issues. Yet other places prefer large PRs with multiple iterations. You might suggest changes to your team's current processes in one of the directions. And consider companies in the future based on your preferences in this area.

Answer 7

I don't know if it happens this way, but why not?

Him: (thinking) I reviewed the first piece of code, let's just submit what I thought then it is saved. Before the next let me get some coffee, go to the toilet, see what all those colleagues want that constantly interrupt me. Sorry code, I'll be back later!

You: Hey colleague, I got your first review comment. Could you let me know when you are finished with it all, then I can look at it and fix it all at once?

Answer 8

I would like to offer a change in perspective on the situation, which may help you cope better with the process in a way that I do not yet see described explicitly in any of the answers.

Your current interpretation appears to be quite milestone-based:

• You work on a task until you think it is done. Then, you create a PR, milestone reached.
• The reviewer looks at the code and leaves review comments, then they complete the review - milestone reached.
• You adapt your code and complete the PR, milestone reached.

Instead, reality with PRs as I experience them works rather in a phase-like manner:

• At first, there is a phase where you work on a task on your own. During this phase, you should implement all the functionality as requested.
• This is followed by a phase meant to fix issues and further refine your work. For this phase, continuous and iterative input from other colleagues is expected. You start this phase by raising a PR for your changes and it ends once all preconditions for completing the PR (which typically includes the treatment of all review comments) are met.

Seen like this, I think the expectation that "a review" is done at any particular moment before the PR as a whole is completed is unrealistic. As long as you are working on the task, others might notice possible issues or opportunities for improvement. You can consider it as some kind of more formalized collaborative editing, where it's still a single person doing the final edits, but everyone having a structured way of asking for changes.

Without harbouring this expectation of receiving a finished review, the process may feel more satisfactory to you. First, you work on the task alone, then together with one or more colleagues providing inout and experience, and in the end, you have produced a useful piece of product that is already built on top of the expertise of multiple people.

Answer 9

Here's an alternative process which doesn't have that iterative quality.

First, decide who's more senior with respect to this particular codebase:

If he's more senior, then view the pull request as saying "please merge this code." If the code is completely off, he tells you why, declines the pull request and you're back to the drawing board. If the code is generally good, but there are nitpicks, he fixes the nits and merges the code. Generally, fixing the nits can be easier than explaining what it is and how to fix it. Make sure you review the changes he makes so he doesn't need to keep fixing the same mistakes in future.

If you're more senior, or you're truly peers, then view the pull request as saying "I'm about to merge this, but please look it over with fresh eyes and your unique perspective." If he has comments, he gets a chance to make them. You review his comments, make changes to your code, or not, and as you're the most senior, you review your own code for nits. Then you complete the merge. If you find yourself or others commiting nit fixes later, let it serve as strong feedback to do better.