How to give feedback on a badly reviewed PR

Question

In my current workplace, I act as lead developer and architect in a team of software developers.

The general policy for merge/pull requests is that 1 person has to review and approve the request. This person must not be the author.

Sometimes code is merged which I personally would not have approved for one or a multiple of these reasons:

• Edge cases were not considered.
• Changes / additions are not designed in a maintainable way.
• There are inconsistent changes: Not all code received the proposed changes – potentially leading to inconsistent program behavior and bugs.
• The change does not achieve the goal of the overarching business requirement driving this work.

My Question: How can I most gracefully give feedback to both the author and the reviewer?

In the past, I have been faced with discontent or resentfulness when pointing out that the review did not meet the quality standards I expect from the team.

Answer 1

Advice for gracefully giving feedback would fall into the "career advice" category of questions, which is off topic for this community, however there are some things you can do from a process perspective.

For the Author

• Missed edge cases: this has been a challenge on every team I've worked on. Usually the best path forward is to point these out to the developer. If need be, add a task to cover those edge cases.
  • Request that the developer review those edge cases with you before submitting a code review if they are new to the team, or inexperienced. At some point, though, you just need to start trusting people.
• Code maintainability: this is a notoriously opinionated area. Unfortunately you just can't identify a common definition of "maintainable." Instead, focus on specific design issues. Make it a learning experience for next time.
• Changes being inconsistent: see the point above. Every engineering problem is a little different. If the structure of the application is fighting the way a problem needs to be solved, refactoring and restructuring things can be justified. This includes violating coding idioms that are common for the project.
• Changes not implementing business rules: People aren't perfect. The code review passed, errant code was checked in. Inform the developer and have them reopen their task or create a new task. They put "soft" in "software" for a reason — it can be changed.

Something I've learned over the years which helps address the items above is to put the team in charge of fixing these issues, and have them deal with the consequences of their decisions. That's how we learn.

On a more personal note, it can be very difficult to accept sub-par work as a lead developer. Even more challenging is accepting that the "sub-par" work was actually the better of two compromises the engineer was forced to make in the midst of problem-solving and meeting deadlines. Giving graceful feedback often means simply giving that person grace. Accept that nobody is perfect, and any issues their changes create can be fixed. Ideally, they should be fixed as soon as possible. Communicate with that person and make a plan without criticizing the person. It's the code that is the problem, not the human.

For the Reviewer

Start by writing a document or wiki page outlining the high-level expectations of reviewers. What should a code review focus on? What kinds of issues should be pointed out? Provide some guidance for people, and know that they will not read and memorize this. Instead, it provides an objective benchmark by which every reviewer is judged. This gives you something to refer to in coaching sessions. Remember to give the reviewer the same grace as the code author.

People aren't perfect. If something slips through, communicate to the appropriate people, and form a plan with them to address the code problem. Again, stay focused on the design issues rather than the people, and your teammates will be a lot more receptive to change.

Answer 2

As a manager or lead developer, your job is to lead, and that includes pointing out to your team members where improvements are possible and/or needed, so your goal is clearly legitimate. Doing this in a way that is both respectful and effective is one of the soft skills that you need to develop in your role. Of course, if employees are not willing to take directions from you, you need to work on that issue as well.

Developing a good communication style is hard work, you might want to mentally switch sides and contemplate what wording you would consider friendly and helpful if you were in your developers' shoes.

Answer 3

The general policy for merge/pull requests is that 1 person has to review and approve the request. This person must not be the author.

Sometimes code is merged which I personally would not have approved

We're striking on a contradiction here. Your system is set up that approval from anyone (other than the author) is sufficient to pass muster, but you're now considering that your opinion is more correct than that of the other reviewer.

One of these is going to have to give.

Given that you're in the leadership position here, I assume you have the ability to redesign this process if you see fit. Therefore, if you think it would be better to do so, you can change the system to only allow either yourself to review, or a more restricted list of people (e.g. senior devs and above).
The smaller the list of approved approvers is, the bigger the load on each approver. Keep that in mind. There is a balance to strike here between quality and availability of the reviewers.

I could also call into question the potentially dictatorial nature of making yourself the only reviewer and becoming the only source of approval. However, I cannot judge the skill level of your developers and cannot judge whether there is reasonable justification for this. My only advice here is to tread carefully.

If you cannot redesign this process due to decisions made above your paygrade, then it's not your place to singlehandedly revoke other reviewer's approvals of PRs. At best, you can raise this issue with whoever put this review process in place and see if they agree that not everyone is reviewing the work to an acceptable standard.

---

At the end of the day, there needs to be a balance. We can't just argue that the person who wants higher standards than another person is therefore more correct. Too low is bad, but so is too high.

Your question does not contain enough evidence that your standard isn't too high for its context. I'm not saying that it is, but I am saying that you can be biased about your own opinions and are likely unable to judge this for yourself.
Given that you are second-guessing other's PR reviews, it is only a matter of due diligence for me to second-guess your second-guessing as well. Consider the possibility that you're not as right as you currently believe yourself to be.

It's not unreasonable to let the current review process stand, and allow for reality to teach your team the consequences of their actions. Maybe it blows up, and during a root cause analysis this PR will be flagged as being the source, which will bring to light that its review was flawed.
Or maybe everything will be allright and there isn't actually an issue.

I'm not saying this is how you have to tackle it, but do consider the possibility that (a) you might be erring towards too high of a standard, and (b) if it's not the end of the world for a mistake to be made, it's sometimes better to wait for problems as they arise instead of trying to proactively defend against every possible problem.

Answer 4

In your role as lead developer and architect, it is your place to double check reviews and enforce code quality. In general, it's ok to have more than one reviewer on a PR. Ideally, nobody owns the code, and nobody owns the review; it's on you to cultivate this attitude.

Rather than viewing your reviews in conflict, your two reviews provide overlapping coverage. If one reviewer accepts and one requests changes, the change is not accepted.

Ask yourself how this got through review, and how it could be prevented. Is it a lack of clear policies? Is it a lack of automation (that is, people are being asked to do and review too much manually)? Is it a lack of time? Is it a lack of training/expertise?

Consider the people and their personalities, they are not interchangeable cogs. Some people come from very strict workplaces which squash their initiative and devalue anything but getting the minimum done; this has to be unlearned, and it's your job to communicate that it is both safe and desirable. Others might be junior developers and are struggling to just get the minimum to work and don't yet appreciate longer-term maintenance issues. Some people might get very prickly and take any critique personally; a back-and-forth in review comments might not be constructive, one-on-one time might work better. Others will be happy to receive the careful attention and teaching from their lead.

If you see the same patterns of unacceptable changes over and over again, you may wish to have a training session with the team.

Do

• Be factual.
• Critique the code, not the people.
• Offer concrete issues.
• Point to existing policy.
• Automate as much as possible.
• Be willing formally Request Changes.
• Be willing to do some work yourself, at least this time.
• Suggest (not require) small improvements.
• Use this as a teachable moment.
• Offer solutions.
• Consider the return on investment.
• Make it as easy as possible to implement your changes.
• Keep yourself open to differing points of view.

Don't

• Make up new policy on the spot, nobody likes a moving target.
• Request changes based on subjective values.
• Require small improvements.
• Set up a conflict between meeting a deadline and making minor improvements.
• Rely on your position of authority.
• Be afraid of conflict.
• Burn goodwill over small, subjective changes.

Edge cases were not considered

This one is the most straight-forward; there's a bug. Offer a clear edge case which breaks the code, possibly with a test. If it's acceptable to commit to someone else's PR (my preference), and the problem is complicated, you may commit and push a test and fix. Then people can review and learn from your work.

You may wish to develop a list of patterns of edge cases that are easily forgotten. For example, slurping in all the data and then processing it risks consuming too much memory; always read and process in chunks.

You may wish to develop utility classes and functions to do common, but subtly complex, tasks.

There are inconsistent changes: Not all code received the proposed changes - potentially leading to inconsistent program behavior and bugs.

For example, they were tasked to fix a bug and did it for A, B, and D, but missed C and E (possibly because only A, B, and D were explicitly mentioned in the instructions). Or they were asked to change the formatting of, for example, how money is displayed but only did it in a few places and missed others.

Similar to missing edge cases, these are demonstrable problems. You can point out the additional places where the fix needs to be applied, or the feature needs to be made consistent.

This can be a teachable moment. You can show your mindset when approaching a task. Rather than fixing only the obvious bug, or only doing the letter of the task, you can show that it's not only ok but expected to go beyond what's in the task and hunt down more places the bug might live, and that consistency of presentation and behavior is valued.

Depending on the people involved, you may wish to do practically all the work to demonstrate how to do the work. Or you may simply give them a sketch and encourage their own initiative, "here's a spot that also has the bug, look for others that are similar".

You may do work to demonstrate better patterns and acceptable initiative. For example, if formatting code is duplicated and scattered around, show them how to consolidate the code into a better pattern, and also demonstrate that this sort of work is not only acceptable, but encouraged.

The change does not achieve the goal of the overarching business requirement driving this work

If this is a high priority requirement, request changes; the PR should not be merged.

Was this made clear in the instructions? If yes, offer a scenario they need to cover (preferably one that can be turned into an acceptance test) and request changes.

If no, make a new issue with the new instructions and mention it in a comment on the PR. The author may consider incorporating the issue into their PR, or they may do it separately. If you wrote the instructions, take responsibility; maybe even pick up the additional work yourself.

If this is a common problem, you may need to consider how instructions are being written. Are issues too large? Are the instructions ambiguous? Are the instructions missing requirements? Is the author not comprehending the instructions?

Changes / Additions are not designed in a maintainable way

The code works, but there might be maintenance issues in the future. They can get subjective; you don't want to be making seemingly arbitrary decisions about other people's code. You also don't want to be seen to slowing people down on trivial changes when they're working on a deadline. If it is within your power, you may reassure the author they will be given extra time.

If there's an existing policy they are violating, point to it and request changes. If there's no existing policy, allow this PR through and discuss a policy change with the team.

As much as possible, use static analysis tools to enforce and auto-correct these maintenance rules. This gives a concrete and tangible set of rules, a concrete way to change them, and it makes it as easy as possible to follow them; it turns the subjective into the objective. If there is no rule for this, open a new PR to add one and discuss the policy change with your team.

You can use curiosity. "This looks like it's doing the same thing as , could they be turned into one method?" You might find out they're actually different. Or "I'm having trouble understanding what this block of code does, could you extract it into its own method? Then it'll also be easier to unit test."

Consider using it as a teachable moment. Often when people are unfamiliar with a language they will write more than they have to. You can leave a comment such as "here's a better way to implement that" and explain why it's better. For example, if I saw code like this:

sum = 0
array.each { |item| sum += item }

I might comment they can use sum = array.sum; it's more concise, easier to understand at a glance, less bug prone, and faster. If they don't now the language well, I might link to the appropriate documentation. If your review system supports it, submit this as a patch so it's minimal work to accept the change. I would not block the PR over this.

Often I will notice ways in which code can be refactored to make it more maintainable. Sometimes it is worth requesting this change if it is egregious, for example, if there is code duplication. For minor violations or complicated refactorings, consider writing a follow up issue; the author can decide to incorporate the refactorings into their PR, or they can open a new PR.

I like having "refactor" or "cleanup" or "follow up" or "style" tags for issues and set aside some time at the end of a working period once deadline pressure is off to take care of them. You can plan some time at the end of a working period to do these cleanup and minor bug fixes.

Answer 5

In the past, I have been faced with discontent or resentfulness when pointing out that the review did not meet the quality standards I expect from the team.

This happens for two reasons:

• The code author already considers the code done and likes neither the suggestion that the code is bad, nor the additional work to get the code up to standards.
• The reviewer reviewed, which is less fun than coding himself. He's being blamed despite doing the dirty work.

Since both the author and the reviewer fail your quality standards, I would push the quality earlier into the code pipeline. Pair programming might help, or a general pointer towards common edge cases.

As for how to implement those changes, I'd strongly advise you to involve your team.

If you formulate a complete Definition of Done document and publish that on your internal Wiki - noone will read it. Instead: Do a Definition of Done workshop.

Or arrange an open-ended meeting where you ask your team what might help them find these edge cases. Be sure to mention and put the agenda into writing so your team can come prepared with examples and suggestions of their own.

Answer 6

The policy requires 1 person review or that at least 1 person review?

You must decide if you're doing this using a privilege that a lead developer has or if anyone can choose to step in and give this feed back.

No one likes being told they're wrong. But if it feels like the team just wants to be better then it's harder to see it as someone on a power trip.

What helps more than anything is showing that you are willing to subject yourself to this level of scrutiny and can gracefully accept this kind of criticism.

Answer 7

There are 2 main and very human reasons why devs have a defensive and sometimes resentful reaction to a code review (or a review of a review):

1. The comment comes after the fact. It might be from conservative bias or not, but they don't want to have to redo part of their work. Of course the larger the PRs the more frustrating this gets.

2. Written comments run on limited communication bandwidth and easily come across as angry, insensitive or rude even if it wasn't the author's intent.

I would suggest reducing the size of changesets and doing as much synchronous collaboration as possible (the former will inevitably lead to the latter, and yes, there's pair or mob programming at the end of that spectrum).

Answer 8

What you're doing is called an audit and is perfectly normal.

In this case, you've reviewed a review and found flaws. Treat this audit just like a code review.

The only real question here is whether you speak to both the code author and reviewer at the same time. This of course saves time, but doesn't make the reviewer look very good.

Another approach would be to speak to the reviewer 1:1, give your feedback, then have them re-review the code with the original author (with or without you present. If it's online, a meeting recording could be helpful here for a second round of feedback or just for confirmation that things were fixed).

---

Code -> Review -> Accept -> Audit -> Reject -> Review review -> Review -> Reject -> Fix -> Review -> Accept -> Audit confirmation -> Final accept

---

To clarify, my suggestion to addressing the author AND the reviewer is to do it indirectly, as a chain. This cuts down on public humiliation (some people are very sensitive to criticism in front of others). That said, time constraints, personal dynamics and experience levels all come into play here and it's impossible to say what is best. For example, if both engineers are inexperienced, your code comments and your review comments might be learning opportunities for both the author and the reviewer, so it may actually be helpful to speak to both parties at once.

Suggested:
OP -> Reviewer -> Author

Answer 9

If code passes a review, that means it’s good enough to merge. That doesn’t mean it’s good enough to stay unchanged.

So if edge cases are not handled and that leads to problems, that’s a bug that needs fixing. Up to you to assign it to the same or a different developer.

If changes are incomplete, that’s also a problem that should be assigned to someone. Say some code was duplicated ten times and supposed to be replaced with a single function. If it was replaced eight times, that can be merged, but someone needs to replace the missing two occurrences.

Sometimes the most efficient and at the same time educational way is to make the change yourself and ask the original developer to review it. At highest priority. That changes the workplace dynamics. Instead of “x is picking on me and just impossible to satisfy” it changes to “my change wasn’t really good enough and x fixed it for me, next time I should do better”.

Answer 10

The best approach is to formalise the PR acceptance rules to try and remove some of the subjectivity.

• Edge cases not covered : Use auto generated test data to cover all possibilities and code coverage tools to check the paths are tested
• Inconsistency : Add a code style checker
• Change doesn't match requirement : Use Gherkin BDD style requirements as tests

Have a meeting about potential rules with the team to discuss ways of speeding up PRs and making them consistent.

Answer 11

Consider adding a Tech Lead Review step to your process. We had this at one place I worked (where I was a tech lead). The main purpose was to check (at the end of a sprint) that all processes were followed (it did not take me a long time to automate most of that exercise). But it also allowed the tech leads an official way to "look over the shoulders" of devs and reviewers in a non-confrontational way. It was particularly useful with less-than-senior folks. In general, if you treat it as a learning exercise (and you make sure that the dev/reviewer doesn't feel "picked on"), it can work well. I mostly hated "tech lead review" day, but it was useful.

Answer 12

Does this answer your question?

"How to write a good answer" is same with "how to present my concerns" otherwise try not making it personal. The easiest way is to get formal and impersonal, you can even prepare a presentation with slides, gather the team and introduce programming techniques to address how the team should address "edge cases", "maintainability", "consistency" and what other concerns are of interest. Then schedule refactorings and assign them to whom ever has to be concerned about it.

Or... just tell them in an informal talk "I am unhappy about the quality of the delivered code. The edge cases could have been addressed more thoroughly by developing..., the over all solution could have been implemented more maintainably by implementing..., the changes could have been more consistent by following...".

Or... just refactor the code and assign the pull request to the author and the reviewer.

Or... just have the team using a static code analysis tool.

Answer 13

I would like to pick up on one specific case:

The change does not achieve the goal of the overarching business requirement driving this work

In my opinion this could be feedback indicating that the ticket/specification writing and refinement process could possibly be improved somewhat. (I will use "ticket" to cover ticket / specification in the following text).

Express the requirement clearly.

I like the team to work together to write tickets with as much available detail and as clearly expressed as would probably enable ANY developer on the team to execute the task. If the details are not yet clear, perhaps that indicates some sort of spike or prerequisite R&D, or further discussion with the client or 3rd party is needed before the ticket can be clarified.

I find Situation, Complication, Resolution to be a format that helps me greatly in clearly phrasing a problem and our envisaged solution. Also include Acceptance Criteria (your testers will thank you for it!)

For example, here your Situation might be we have a team of mainly junior developers, and a PR approval process that requires only one approver. The Complication is that work is being released that does not meet the business requirement. The Resolution is what you are trying to find.. (e.g. Adopt the SCR framework for tickets, require 2 reviewers, including one senior/domain expert, and all the other good suggestions in the answers here).

Ensure the requirement is understood by the developer executing the work.

I would also ask the team at (sprint) planning (or the point that the work gets into the schedule and is assigned) "do you feel there is enough information in the ticket for you to be confident in doing the work?". If anyone says no, ask why and use that feedback to improve the ticket, or it can't go in the sprint/work list yet.

Of course, when actually executing the work, it may well take further back and forth with other technical staff, team members, Product Owner etc. to actually update the ticket content and complete the work. Totally normal, and I assume either Scrum dailys or other regular contact enables developers to interative converge on the needed information/ solution. If appropriate, include a list of "who to contact about what" in the ticket.

I also find it helpful (if possible) to document all communications in ticket comments, with firm decisions being used to update the ticket itself. In an Agile system the ticket may be the only specification, so I think that it should be kept up to date.

I think that is most important that everyone understands extremely clearly WHERE we are trying to get to, rather that try to micromanage the HOW in any great detail. Because we are going to have to imlrovise somewhere along the way! This may be more or less tricky given the problem domain and experience and aptitudes :-).