56

For context: my productivity as a software developer is tied to delivering my work, and delivering my work is tied to having my code reviewed and approved by co-workers.

But lately I've been having trouble getting co-workers to start the code review. For easy ones (one-line changes) it would takes a few hours, which is fine, but I've had critical tasks that would block others taking multiple weeks. It's never too large or particularly hard to review – anything more than 300 lines would be broken down.

This is the second time this has happened in my career.

At a previous company, I had a manager would have to intervene from time to time. Once he rage-merged all my Pull Requests that were open for more than two weeks because the rest of the team was not reviewing my work.

I routinely make an effort to prioritize reviewing other people's code and I'm always the top reviewer in my team according to our reports... both in time and in amount of reviews/amount of lines of code reviewed. When it comes to juniors I try to make an effort not to be too hard or too serious, I always give positive feedback as much as negative, and I always try not to leave too many comments...

In both teams I've been among the most senior members, and never had bad feedback about the quality of my code. In one of the teams I was the "go to code-quality guy".

Is the problem me? I have friendly relationships with my co-workers and hang out with them outside the office, plus I never had fights or anything like that, and I make a lot of effort not to come out as standoffish or cocky. I rarely cause bugs and when I do I stop everything I'm doing to fix them or help the other person.

I have managers complimenting me, which is weird, because I feel as there is a rift between me and the rest of the team.

This has made me feel that me and my code are deeply unwanted by the team.

Is there any solution to that? Is it all in my head?

  • 1
    Comments are not for extended discussion; this conversation has been moved to chat. – Neo Mar 7 '20 at 3:51
  • Did you considering asking your manager to pay a freelancer to review your code? I could even be that freelancer (mail me to basile@starynkevitch.net and look at my resume) – Basile Starynkevitch Mar 8 '20 at 19:49
  • Are you standing out "in front" of your team? If so, some teams react badly, even if it wasn't your intent. While plenty have offered "they probably think you don't need it / they probably are afraid to review," there's another possible reason. It slows you down. – Edwin Buck Mar 11 '20 at 6:26
  • @Bohemian That's a good strategy. Manager actually agrees with your suggestion. – deutsch-koder Mar 13 '20 at 12:57

14 Answers 14

94

I noticed in your post things that could be linked to that situation:

  • You are the top reviewer of your team
  • You are also one of the most senior member
  • Your individual productivity is monitored based on produced code

Therefore

  • Your team might be forgetting that they need to do code reviews since you do so much of them
  • Your team might think they don't have the skill necessary to "judge" you
  • Your team might be thinking they have to focus or prioritize their code production instead.

You shouldn't be afraid to ask your colleagues for review and for potential reason they don't review your code. My approach would be to have a conversation with colleagues in small committee or 1 on 1, because I feel like there would be easier to be honest there, but you could also start asking in meetings e.g. retrospectives and such. Since you have metrics you could ask people that do the least reviews, and be reassuring about the fact it's both easy and appreciated.

Eventually, after you've asked several times to people, they will take good habits of taking a look at reviews and review things by themselves. We often ping ourselves in chat for pending reviews and the load is quite fairly balanced now.

  • 48
    "Your team might think they don't have the skill necessary to "judge" you" This was exactly the point I was trying to write an answer around, so +1 instead. – Player One Mar 5 '20 at 13:03
  • 1
    The first point is a good one. By the OP always being a quick reviewer, it's easy for other developers to see "oh, hey, someone reviewed it already, I don't need to" and falling into a "I don't need to review, someone else will do it" mindset. Best thing to do is assign reviews instead of just general open requests so it comes up as part of everyone's regular tasks (and can be tracked so managers can see who isn't doing them). – pboss3010 Mar 5 '20 at 13:16
  • 4
    Also, remind them that a Code Review is a good opportunity for them to learn from you, and that any question is helpful. Do this in a way that doesn't feel patronizing, like adding a "that's one of the reasons I do so many reviews" (which is most likely true) – Blueriver Mar 5 '20 at 21:26
  • 1
    My first feedback to whoever organizes this team's processes would also be that the process gives the wrong incentives by evaluating people by their implemented features. If you feel you need such an evaluation (and I typically scorn at something like that), then at least make sure all relevant parts that someone needs to do are actually part of your metric! I.e. your reviews need to count as well. That would be nicely 'gamifiable': a feature only counts to your "points" if there is a review that "unlocks that slot". – Frank Hopkins Mar 6 '20 at 2:06
  • 7
    "Your team might think they don't have the skill necessary to "judge" you". Remind your team mates: It is the code that is being judged, not the coder. – Peter L. Mar 6 '20 at 5:30
23

I think the problem here is that team members are each expected to deliver their own work, instead of having a common goal of delivery.

If each engineer has her own piece of code to write and deliver, and is measured on achieving this (and not on achieving a team goal), then of course there is no incentive to review other people's code changes.

I'm not sure what you as a single person can do to change this, though – this needs to be approached from an organizational point (i.e. your manager, or a product owner or similar). The team needs to have clear priorities on what is most important, and then those delivery bits will need to be reviewed before working on other code.

  • 1
    You can always provide feedback and start a discussion. Often you will find that others share your sentiment to some degree if a process is flawed. They just might handle it differently. OP might also cut back on doing reviews so the whole team sees that somehow they all need to agree how to best distribute that work load, as everyone will suffer when no one does them. But in general spot on, this seems to be largely a team vs everyone for themselves problem. – Frank Hopkins Mar 6 '20 at 2:17
18

Go to your coworkers' desks and ask them if they are available for a code review. For short ones they will most likely agree, for longer ones they may ask to do it later, take the opportunity to specify a time slot for your review.

If you really need to you can formally set up a meeting, it can be usefully to both ensure the code review take place and prevent you or you reviewer being bothered by other people.

  • 13
    Ask someone specific to review, yes. But don't go to their desk unless (1) you need to pair while reviewing (reviews can usually be done alone), (2) you've already tried asking them using GitHub, IM or during standup or (3) you're specifically trying to irritate them or get on their bad side. – NotThatGuy Mar 5 '20 at 23:21
  • I get your point. I do prefer to do review in pair because it allows discussion and prevent going back and forth through a tool when there are questions or suggestions of improvement. – JayZ Mar 6 '20 at 9:10
  • We have daily standups and one of the things we do on them is check the list of pending pull requests. Anyway, asking a coworker personally (in the office, or using a messenger app) is pretty common when we want to deliver a feature fast. Also it's nice if people add only relevant approvers to pull requests, otherwise approvers start to ignore them. – Sulthan Mar 6 '20 at 18:06
7

Firstly, don't internalize the lack of code review as being a problem your team has with you. It seems like you're thinking that the lack of code review is a personal slight against you, which likely isn't the case. Sometimes people are busy or they don't see the benefit of reviewing your code. It isn't an attack.

Second, if you see a need for code reviews and the team isn't committing the time, speak to your manager. A few places I've worked had a set time every week for code reviews, and we would knock out a few of the more critical reviews as a team for larger or higher priority changes. Smaller ones we could review in pairs as our schedules permitted. But we never allowed code to be merged without a review.

Speak to your manager about establishing a code review cadence that works for your team, and be open that your manager might not see it as an important change to implement. To some managers, code reviews aren't important until they see how it can improve the bottom line.

7

It's not you, the issue is the team culture. By by-passing code reviews, the team does not follow software engineering's practices and don't understand why they are important. This is a critical issue that needs a change ASAP. These points need to be tackled:

  • Why it's important.
  • For who? and who should do it? I would clarify that code review should not only be done by seniors and that seniors also need their code to be reviewed. I have seen teams who introduced git enforcement rules like protected branch that needs two reviewers.
  • How to code review and how to give appropriate feedback.

The expectations should be clear. These points need to be presented to the team (E.g. meeting, lunch & learn). Change is hard, be prepared and seek allies.

  • I believe this is quite clear to the team already, considering code reviews are already a requirement for merging anything across the whole organization. – deutsch-koder Mar 5 '20 at 14:38
  • @deutsch-koder the team might know that they are "required" but they might not be motivated to do them. Part of that might be that they don't see the sense in the requirement. In general or because they are wrongly incentivised to not care about them because they are not part of their performance evaluation. While I don't agree with "presenting" to them how it all is (that might well come across as patronizing), you might need to have a discussion how you want to handle them properly, what your process should be, how you visualize that and you might want to change your incentivation. – Frank Hopkins Mar 6 '20 at 2:12
4

I was in a very similar situation. Quality guy, managers complimenting, meticulously taking care of delivery, good relationship with people outside work.

It was on top of my priority list to be a great performer. One day a new guy in my team told me that he heard that I'm an a**hole but everybody loves me. Honest 360 degree feedback. :)

What I realized then was that I was so obsessed with perfection that people were my second priority. Still a high priority, I did whatever I could for the people, except sacrificing quality.

People feel it. Even if it's not conscious on my part, I walked tall like quality was my second name. I realized that this makes people feel secondary or inferior.

I put people as my first priority for real immediately, because that was my intention anyway, I just did not see that I'm not there. Life improved a lot, people built so many new skills, team got great, quality was strong and steady.

This might not apply to your situation, I just leave it here, maybe it helps.

  • 2
    I was like you. A pain in the neck that people appreciated doing their reviews. For code that I really knew I would find dozens of problems in a large review that no one else would notice (when I went last deliberately). The trick is to make sure that you remember to question "our code" not "your code" or "you". It goes over much better that way. – Sinc Mar 6 '20 at 18:35
  • What was actually involved in switching your priority from quality to people? behaviour-wise? – Bwmat Mar 7 '20 at 1:07
  • First I used the methods that we use in school to succeed individually. But that did not work with a team. I acted like a schoolmaster and that caused a lot of friction and did not feel good and it was not productive. Empowering people is where the magic happens. Totally different universe and it feels great. Help others to success on their journey and the team will flourish. :) – takacsmark Mar 7 '20 at 6:55
  • 1
    Could you elaborate on the diff between 'acting as a schoolmaster' and 'empowering people'? – Bwmat Mar 7 '20 at 20:51
4

What has worked for me is to explicitly assign someone for the review. In our current project, we even go so far as to reassign the issue in JIRA to the reviewer. If you can't assign this to another person, assign it to the one who can (the project manager). It doesn't matter that the person actually does the review, it matters that the person takes care a review is done.

Assigning a specific person means that this is now, traceably, a todo of that person. I find that even for me, who does most reviews anyways, it helps if review tasks are assigned to me - it makes it clear that a task is in a state for review, and that progress is blocked by me right now / that I am responsible right now.

  • This, absolutely this - "If it's everybody's problem, it's nobody's problem." When you assign it to someone you make it their responsibility, and something they can be called out on in daily standup if they're not fulfilling it. – Ian Kemp Mar 6 '20 at 10:13
  • explicitely -> explicitly – Faheem Mitha Mar 8 '20 at 19:32
  • @FaheemMitha Thanks, fixed – kutschkem Mar 9 '20 at 7:15
3

Consensus

You can't get team members to review your code if your team doesn't agree that code reviews are valuable. I'd say code review is one of the three things that most developers don't do enough of. So the first step is to simply talk about code review in a meeting with your whole team (including managers). Ask them what they think about it, whether it's valuable, and how often it should be done.

I'll bet good money that at least half of them think it's a waste of their time. If so, your first step is education. You need to pull up reviews where valuable comments were made, especially if someone caught an actual bug before merge (you shouldn't sell CR as a bug-finding tool, but it is a nice bonus when it happens). If your coworkers lack experience, this might be a hard sell. Even so, it's something you should gently raise as often as possible, so they eventually absorb the idea that CR -> better quality -> better code -> fewer bugs -> better Quality of Life for devs.

Enforcement

Once you get the team to at least pay lip service to the idea that CR is valuable, you then need to make a mechanism for enforcing it. Ideally, your version control system does this (trivial to configure on GitHub, for instance). If nobody can merge without getting a CR, then hopefully everyone is bottlenecked on it, and is thus incentivized to clear out the backlog. When this happens, you need to do something which may at first seem infinitely difficult for you: you need to do nothing. There is an average number of PRs produced per dev, and each dev therefore needs to perform that number of CRs to have an equal distribution of work. You need to estimate that number and do no more than that many CRs.

Soon, everyone on the team will complain that there is a huge backlog of PRs waiting to merge, and you need to make it clear that the whole team needs to carry the load of reviewing them. Gently remind them that they agreed on the value, and resist any calls to remove the CR lock on merges. Most likely, they will do CRs grudgingly and sporadically. To help assign accountability, I created a tool for my team which did a very basic round-robin assignment of PRs to devs. Everyone could see who was not doing reviews, and everyone knew who to talk to if their PR was blocked on review. Just having this visibility helps enforce the desired behavior.

Also, seniority really has little to do with this phenomenon. I see it across devs of all skill levels. Many times it is the junior devs who grew up in a more modern software culture who embrace code reviews, while senior devs who are used to merging without review resist it. So, really, it is just an individual preference which is unfortunately pervasive throughout the industry.

Expectations

Finally, you need to get past the idea that CRs are for finding defects. Code review should be primarily about distributing knowledge. Just having someone else read your change makes another person on the team aware of the change, and more likely that the team notices potential design conflicts with concurrent or imminent changes. It's also for education. Junior devs should get just as much out of performing a CR as seniors, but of a different nature. You should teach the juniors that if they have no critiques of the code they are reviewing, then they should ask questions. Surely they are learning something new here and there, or they see something unexpected that was done differently than they would do it. Asking these questions and getting good answers helps them learn which practices are specific to your company vs. general software engineering best practices.

If anyone expresses reluctance or hesitation to perform reviews, especially of your code, then just offer to pair-review with them. Tell them the things you look for, and ask them to explain your code to you. If their explanation misses something interesting, ask leading questions to highlight your point. If they fail to understand some bit thoroughly, then ask them to put a question on the review to show that your code was not as self-documenting as it could have been. Then go through one of their PRs or someone else's, and demonstrate the same principles. Show by example that a CR doesn't need to be intimidating and that it can be a valuable learning experience for everyone.

1

You mentioned in the comments that you're working on some more complicated stuff than the rest of your colleagues.

This looks like a double danger: your code doesn't get reviewed, and other people don't know how the advanced topic works. If you got hit by a bus, the team would be in trouble.

That could also be the angle to deal with the problem though. Have a talk with your supervisor about how you think this is causing the team to be vulnerable, and request that 1-2 colleagues be chosen to get "initiated" in the difficult stuff you're doing. That makes them the logical choice to review your code, and it also improves your bus factor.

  • That's a good point. That's a much bigger problem than the lack of reviews. But to be honest, I'm doing exploratory programming, and I expect some effort from them as well. I'm always available to explain things and I made a point to tell everyone that they should "tap in my shoulder even if I look busy". – deutsch-koder Mar 13 '20 at 12:50
  • Btw I followed trough and talked with my supervisor and he also agrees that we need to share knowledge. – deutsch-koder Mar 13 '20 at 12:55
1

This seems like a process and management issue. First, you work on a team, so you succeed as a team and you fail as a team. If deliveries aren’t met, that’s a team failure. In the past if we had issues with code reviews and peer testing, and we have, we as a team made contracts to agree that all reviews and peer testing had to be done before starting a new dev task and anyone caught breaking the rules got called out. Other times we just assigned those tasks in a round robin manner. You, your team and your manager need to solve this together, but never be afraid to call out an issue especially if deliveries aren’t being met. Be upfront, be direct, but be polite. Also, don’t be afraid to ask people what the issue is. You’d be surprised, most of the time they’ll actually just tell you.

0

It may not be you, or the process, or busyness. It may simply be that a lot of programmers don't like to do inspections. Some don't believe in them, some don't want to take the time, some know that they aren't that good at them.

There are people like you (and me) that are very willing to dedicate a decent amount of time to inspect a change correctly. There are reasons we do that. I can't say your reasons, but mine include things like:
I inspect better than most designers I've ever worked with,
I spell and read better than most so even your comments will be corrected,
I'm defensive about other people making bad changes in the system that might affect me, or even worse our customers,
my early history included organizing and chairing the old fashioned sit down group inspections which gave me a remarkably broad exposure to our code so I recognize the value of seeing other people's code.

Learn, protect, team. Useful values from participating.

You probably know who the other best inspectors are. They likely share your interest in inspecting, at least a little bit. See if you can figure out why they do it or why they are good at it, and appeal to those instincts. "I've made some changes in this tricky bit of code and I need someone good to inspect it." Or if they are just team players "I need this inspected in [a specific time frame]. Could you help me out?"

0

I completely agree with what has been said regarding reluctance to review complicated stuff and lack of assigning somebody to be responsible - and I've seen nothing in your question to indicate that you and your code is unwanted by your team! I rather think that your team has no clear processes on how to handle reviews.

We have talked a lot about reviews on my team. Maybe some of our conclusions could be helpful to you and your team:

  • What do we want out of a review? We are not (primarily, at least) looking for the small details. We want to catch pitfalls and bad architectural choices and the like. We also want to exchange good ideas and improvements we might be able to spot, but this is secondary.
  • Because this is what we want, it would take way too long to do the review without the author. It would be like having to think all the authors thoughts all over, but without the benefit of gaining experience during development!
  • Even if the commit is fairly simple, it doesn't have to be scattered across very many files before it becomes hard to figure out where to start and where to end - especially if this is not "your usual area of the code"
  • We are experienced developers, and honestly, we usually have a pretty good feeling about what is danger area and what is probably trivial in what we just did.
  • Reviews are an excellent opportunity to share knowledge!
  • Our solution: The author must ask one or more team mates to come for a review. This can be at the desk or in a meeting, depending on the scope. The author must present what they did, what choices and compromises where made and why - and then the team mates provide feedback. This way the responsibility falls on the author and the reviewer doesn't have to understand everything on their own: they have it presented by someone who has an overview and can answer any questions that may pop up along they way.

I will add that, on a personal level, we are a very well functioning team with a high level of trust. We do not take feedback personally and we present it in a positive, disarming manner. Also, if we work on complicated stuff, we spar and discuss along the way. This way we have a conceptual overview of what the others are working on even before we have to review the actual code.

Even if your team does not come up with a shared strategy for reviews, you can take ideas from this, if you think they will work for you: spar during development, invite team mates for reviews where you present - and do make sure you tell them about doubts and compromises you've had along the way. They may actually be a little intimidated by you if you are very good, so if you show them you have doubts too, they may become more at ease with the reviews.

0
  • Bring that up during regular team meeting (if you have a daily standup or similar, for example):
    • "Whatever task I'm working on is code complete, I need to get it reviewed."
  • Right that moment, find someone to do it:
    • "Who can take care of it/who has time/who can I assign it to?"
  • Get verbal commitment, if possible a timeline (before lunch, by 3, later in the day, tomorrow, ...)
    • "Thanks, I'll merge it afterward, so we can meet such and such deadline"
  • Make that visible in whatever system you are using (e.g., in bitbucket, assign the person as reviewer)
  • Bring this up again during stand-up if your code hasn't been reviewed.
    • "Guys, this is blocking me, it needs to be reviewed otherwise feature A can't be shipped next week"

Later, have a discussion during retro (if you have that) or an ad-hoc meeting to explain that code review is part of the work of everyone, that it's a good way to know what others are working on, to learn how they work, and that it's necessary for the team and in order to deliver anything, and so on - I assume you know what code reviews are good for :)

0

I've both had and experienced this problem - largely the root cause being either or both of

  • you produce a large volume of arguably fine code (meaning they have no real input for it beyond trivialities)
  • your code style or the source language is very unfamiliar (meaning they are not confident in their review, worse being the need to read piles of documentation to understand it)

Even if there is a legal reason to require reviewers and not some departmental whim, don't worry directly about their reviews, but instead provide tools to help them review your code - this will give them confidence in the sound-ness of your code as a whole, rather than combing every line for absent errors

  • write tests for your code and request reviews on the tests .. try to write a good description around the purpose of the tests
  • ask for and work with your boss(?) to create work items for tests against specific functionality
  • if possible, collect coverage to prove that the tests do cover all the branches you expect

This will make it far easier for your coworkers to review what you've written, because having contributed to the tests and seeing the good result, they'll have confidence it works!

Additionally, try meeting regularly to have all members show something they thought was neat during the past week(?). Go last and be sure not to hog the whole time - there will be more and they should be legitimately interesting, not a time to show off or "teach" about your thing. This will help familiarize everyone with each others' styles and tools which can be used to understand and develop with their logic.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .