38

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 ore 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.

4
  • 8
    Are you more interested in reprimanding the specific people so they take more care; or adjusting the process to reduce chances someone does anything similar again? Approaching it as the latter makes the conversation less personal, can include whole team and can easily floated in a retrospective
    – Alex
    Mar 20 at 21:13
  • My primary target is to 1. have open issues with the reviewed code fixed (example: address remaining edge cases, adjust design to objectively improved version) and 2. allow both author and reviewer to learn what were the vital issues which should not have been merged in the first place.
    – FabianTe
    Mar 21 at 10:05
  • If an edge case was missed, sometimes they are easily added in an additional PR.
    – Mast
    Mar 21 at 16:31
  • I could have sworn there used to be an option to suggest sending this to another SE like Workplace... maybe I just don't have the Rep needed here
    – Mars
    Apr 1 at 4:46

12 Answers 12

43

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.

5
  • 17
    "Start by writing a document or wiki page outlining the high-level expectations of reviewers." I would like to put some emphasis to that. That is a very, very important point. People usually take it way better if you tell them what you expect from them (e.g. by an agreed on "Definition of Done") before doing something than telling them after they are (according to their believe) "done".
    – AlexV
    Mar 20 at 10:44
  • I second the wiki idea of what is done. What about bringing this up in retro as well? Mar 20 at 13:31
  • 2
    @heretoinfinity: retrospectives are good places to broach the subject. Oddly enough I literally just got out of a retrospective where I brought up an issue with pull requests on my current team. Remember to work with the team to solve the problem. Try things out, and change next sprint if need be, even if the proposed solution isn't what you wanted. Nothin' says like doin'. Mar 20 at 14:25
  • 2
    pull request templates are great place to list those expectations. Adding a checklist of things are reviewer must check through within the PR acts as a reminder for every PR and prevents those expectations from being hidden away in a dusty corner of a wiki
    – Alex
    Mar 21 at 19:26
  • To add onto this, FYI, Google's Engineering Practices documentation, including code review guidelines, are publicly available under a creative commons license. I'm not saying you should apply them to your team exactly as-is, necessarily, but you can borrow pieces or link to them from your own wiki and avoid re-inventing the wheel.
    – Wildcard
    Mar 29 at 15:48
29

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.

4
  • 5
    OP already seems to know that he needs to do what this answer says. He is asking how to do that.
    – AnoE
    Mar 21 at 9:21
  • That's what I try to express in the last paragraph - putting yourself in the others' shoes is one way of finding acceptable ways of expressing oneself. Of course that only works when there are no major differences in your own and others' perception of what is said. Mar 21 at 9:26
  • 2
    IMO: The fact that OP asks this question indicates he agrees with your first paragraph (so that adds nothing for OP). That its a communication problem is pretty obvious, because he wants to express his issues (eg communication). So really this answer is just "Think about wording and how they might be interpretted". Maybe add some more specific suggestions to improve this answer?
    – Martijn
    Mar 21 at 12:19
  • @AnoE Thank you! This isn't an answer, it's the premise for the entire question.
    – Mars
    Apr 1 at 4:48
17

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.

13
  • 1
    Good points! Pointing out that one of the two options may have to give seems reasonable to me. To clarify: I am very aware my view may be biased. That is true for everyone everywhere. In my context, the author + reviewer have a very junior level of understanding for the software they are dealing with. There is a lot of legacy code which gets overlooked or purposefully ignored. Reviews around that legacy code is mostly what motivated my question. My task is to prevent naive ignorange of legacy code which is cause for most bugs (in my context, again).
    – FabianTe
    Mar 20 at 7:06
  • 7
    @FabianTe sounds like you need to change the review process so that at least one person with a good understanding of the legacy code has seen the review before it goes through :)
    – Erik
    Mar 20 at 7:53
  • 2
    So it seems. We have not yet established anything like explicit code ownership across the codebase.
    – FabianTe
    Mar 20 at 8:54
  • 1
    @FabianTe: Yeah don't take my answer as a suggestion that I think you're in the wrong, I'm just leaving the door open because you'll likely have to deal with the perception that you're overriding "other valid opinions" (regardless if correct or not) because those people can also succumb to bias. I've been in your position before and restricting the reviewers, even if temporarily, can help stem the flow of technical debt. Good luck!
    – Flater
    Mar 20 at 21:24
  • 1
    @Mars: Merges can be rolled back if they're on top. And while it's not impossible to rollback and then reapply the changes on top of the ones you wanted to remove, step by step this process is getting hairier for a shared branch between devs. This is why the PR happens before the merge, to not have to deal with the complexity of rollbacks. I agree with your point that if the goal here is to only review the reviewer and not suggest changes to the actual reviewed code, that doing so post-merge is absolutely fine, because that is a "read-only" process in terms of the code in the repo.
    – Flater
    Apr 2 at 22:17
12

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.

3
  • +1, but "The change does not achieve the goal of the overarching business requirement driving this work" is handled too strictly. PR is not an issue in the tracker - it represents a unit of work. The unit may be just one of many units required to reach business goal. So as long as PR is self-consistent and is related to the business task (even if does not completely solve it) it should be merged.
    – Basilevs
    Mar 22 at 9:58
  • @Basilevs "It depends". The assumption is the PR claims to complete the issue. If one is doing continuous delivery and always ready for release, don't merge anything that's not ready for release. For example, if the issue says "add feature X and make sure it works under high load", and the PR claims to close the issue, but it crumbles under load, don't merge that. A solution is to hide the incomplete feature behind a configuration flag, default it to off, and say the PR only completes some tasks in the issue. OTOH, a partial bug fix is more likely safe, but don't claim the issue is complete.
    – Schwern
    Mar 22 at 19:18
  • If it crumbles, it is not self-consistent. Each PR should leave product releasable, but that does not mean all business needs are met.
    – Basilevs
    Mar 23 at 10:33
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.

1
  • I figure that most often the generated feeling was "Why do I have to change this now, after someone has approved it - i am done". From my perspective the whole thing is not done since it lacks quality / correct test coverage or similar. Assuming that the author is less proficient that the reviewer, I feel like I should address the reviewers review first in a personal discussion since my critique is mainly that the original reviewer missed vital points.
    – FabianTe
    Mar 21 at 10:08
2

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.

1

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).

1

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

-1

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 how 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.

-1

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.

2
  • 4
    I think that regarding edge cases, the OP means that neither the author nor the reviewer though of an edge case that they should have considered. Not that edge cases in the code were not covered by tests. If there is no code for the edge case, no automated tool can tell you it's missing.
    – Bergi
    Mar 19 at 20:29
  • ok let me edit that, i was more thinking if you didnt cover nulls for exmaple a test should fail or code coverage be missed
    – Ewan
    Mar 20 at 13:20
-1

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.

-2

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 :-).

2
  • 1
    The goal may be achieved in a following PR. This is not a reason not to merge PR, this is a reason not to resolve a task in the tracker.
    – Basilevs
    Mar 22 at 10:02
  • Agreed. Work can and will occur in multiple tickets. On reflection, I probably expressed myself less than perfectly. But I don't think I said not to merge the ticket? I did say not release (work that doesn't meet the requirements)
    – kpollock
    Mar 23 at 12:56

Not the answer you're looking for? Browse other questions tagged or ask your own question.