-1

Background: As a developer what should I expect as code review feedback? What if I perceive it as rude, condescending or otherwise toxic?

This developer has become so frustrated with their code review process that they are now escalating the matter to their manager and HR. Their complaints are that their reviewers are under qualified and are often asking the same questions over and over again, even questioning decisions that were made in prior meetings. They seem to be having an unusually emotional response to this, even describing mild panic attacks.

I know a lot of people dislike code reviews and sometimes take criticism personally, but I've never heard of something this extreme.

Ideas I've had:

  1. Discipline them and give them very clear feedback that this is part of their job and the expectations are that they will help other, less experienced, developers understand their code. (I don't think this will change their behavior based on how remarkably strong their feelings are)
  2. Give them some sort of preferential treatment by either having them only get code reviews from someone who is willing to deal with this behavior. (I think this will cause huge problems in the culture of the development team in that others will resent this and it will only encourage the behavior)
  3. Fire them. (I hate this option. It doesn't really solve the problem, it just gives it to their next team.)

I'm not going to build a team of all senior engineers to satisfy one person's complaints, there are several mundane tasks that can be handled by less experienced developers; I'm not willing to pay someone six figures to handle those simple tasks (even if I could afford it, I'm not sure I could find that many of them). Part of our code review process is to improve their skills and let them understand what's going on around them, so abandoning code reviews isn't going to happen either.

closed as off-topic by Dukeling, gnat, Snow, scaaahu, Rory Alsop Jan 15 '18 at 12:59

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions asking for advice on a specific choice, such as what job to take or what skills to learn, are difficult to answer objectively and are rarely useful for anyone else. Instead of asking which decision to make, try asking how to make the decision, or for more specific details about one element of the decision. (More information)" – Dukeling, gnat, Snow, scaaahu, Rory Alsop
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 5
    "It doesn't really solve the problem" Of course it does. You're not in the business of training people to be good developers. As a manager you're in the business of creating and maintaining a good team. That being said, is there any truth to this person's claims? Are your code reviews not being run properly or is simply the case that the code reviews are pointing out problems over and over again because the developer keeps making them? And at the risk of turning this into more of an answer than a comment: realise that you should only do #3 after several instances of #1. – Lilienthal Jan 13 '18 at 19:02
  • 4
    Wait, hang on. I read over the background link. Are you talking about a hypothetical situation where you're managing the author of that linked post? Because if so then you wouldn't be able to answers my questions in the comment above and there is really very little point in asking the question. You'd just be asking "how do I manage a person's performance/behaviour problem" which is Managing 101 but too broad of a topic to cover here. – Lilienthal Jan 13 '18 at 19:23
  • 5
    Some other (not very good) options: 4. Set constraints for what is and isn't allowed as part of code reviews. 5. Don't let any junior developers review code of someone non-junior. 6. Hire the type of people who wouldn't give the type of feedback which might possibly offend someone. But it seems that which option to pick would be entirely up to you. – Dukeling Jan 13 '18 at 19:46
  • 2
    All your three suggested options are harsh and condescending, I'm frightened there was no option "Actively listen to devpr's complaints, and help reframe them into constructive change: 'How can we improve our code review process/ software process? Eliminate repetition? Gather and formalize our knowledge base/ codebase? Improve documentation?'" – smci Jan 13 '18 at 19:54
  • 3
    "even questioning decisions that were made in prior meetings" Well, if that's true, who's keeping (concise) minutes of those meetings? in the same documentation system as code reviews? If noone, appoint someone ASAP. Again, that's a legitimate constructive complaint/improvement suggestion. Code reviews are not back-door new-hire training sessions. – smci Jan 13 '18 at 19:56
19

I'm pretty sure this retreads ground from previous questions, but having been in the shoes of both sides, I'd give the senior developer some presumption of clue, and work with them to convert their complaints into constructive actionable improvement recommendations, and have everyone follow through on those process improvements, and monitor with each party involved if they're happy things are improving:

  1. To be frank, quite a few code reviews are “rude, condescending or otherwise toxic”, if the people doing them are underexperienced, don't communicate well, petty, clueless, lazy, there's a total lack of guidelines, software process, documentation, written minutes, follow-through... it's your job to convert complaints into constructive improvement. I don't get why you seem to automatically doubt everything your developer says and dismiss them as a crybaby, even if they're not saying it in the optimal way, or to the right parties. For the love of God don't expect HR to articulate constructive improvements to your software process. In the history of software I've never heard of this occurring. Unless the code reviews are kindergarten-grade name-calling.

  2. Your three suggested options are limited to being harsh, coercive and condescending, I'm frightened there was no option "Actively listen to developer's complaints, and help reframe them into constructive change: 'How can we improve our code review process/ software process? Eliminate repetition? Gather and formalize our knowledge base/ codebase? Improve documentation?'". That's a criticism of your listening style. So you think they should have been able to put up with it because you could... beware of condescending, we all have our weaknesses, check your ego... there are presumably things they can do easily which you might find impossible. If you honestly find it that hard to interact with them, go talk to them privately and clear the air. Constructively. Sometimes some of my more annoying coworkers have taught me useful things. Even if I had to decode their ball of emotions.

  3. "even questioning decisions that were made in prior meetings" Well, assuming that's true, who's keeping (concise) minutes of those meetings? in the same documentation system as code reviews? (JIRA? git? Bugzilla? wiki? Mondrian? GoogleDoc?) If no-one, appoint someone ASAP. If someone was doing it but badly, tell them to do it properly, or reassign the ask (but beware of rewarding bad behavior). Again, that's a legitimate constructive complaint/improvement suggestion. Code reviews are not back-door new-hire training sessions. Don't let new-hires push their responsibility for learning onto senior developers and into code reviews. Also, do you have meetings that don't result in decisions, or action items? That's another common organizational antipattern. Is a comment a decision? Is a group vote legitimate? When is the majority wrong? etc.

  4. one powerful suggestion (esp. when nerds communicate) is to reframe questions/criticisms to focus on things, not people, and in particular to depersonalize queries/disagreements: e.g. *"Why does your code do X? Why did you implement Y as A instead of B?" -> "What are the reasons for lines LLL doing X? Why is it better to implement Y...?"

  5. As a general process thing and to teach everyone that your process is community-owned, have the junior guys (esp.) start reading a bunch of books on improving the coding and reviewing process, e.g.. "Code Complete: A Practical Handbook of Software Construction, Second Edition", esp. Part V. Have them make process improvements. Make them document repetitive questions (have them start a wiki). And so on. Ask the developer if these improvements are helping.

  6. Also privately ask your developer (and their manager) to always try to reframe complaints into process change requests ('active listening' is one of many techniques. Basic empathy always helps, too.) They'd probably benefit from some book or course. If the situation got escalated as far as HR, that also tells me the manager involved was not good at handling this sort of thing. So, you all need to learn from this. Don't hang this on developer.

  7. It's often hard to disentangle technical problems from people problems, and more so the longer they go unresolved and the more unfairly-treated or dismissed people feel. You all should collectively never have let things go so far...

  • 1
    +1 for your 5th point alone. Never make a code review personal. Keep it about the code. – candied_orange Jan 15 '18 at 7:31
  • 1
    Thank you for this answer. I am in the camp that is not yet convinced code reviews will achieve anything positive in the context of my team and have been unable to put a finger on why, but your answer has helped me understand how we can get to a place where code reviews are meaningful and a good thing. – Thomas Carlisle Jan 15 '18 at 15:02
6

Took me a while to grasp what this is all about: You have a senior developer who writes code, and there are no complaints about this code. Code is reviewed, as it should be, and often the code of the senior developer is reviewed by much less experienced developers.

And what he complains about is that these less experienced developers are clueless, ask the same questions again and again, can't remember what was said in meetings, and are generally a pain to someone who doesn't suffer fools gladly.

This sounds bad. Your problem is that you have devised a method for training your junior people that is absolutely inappropriate. The role of a reviewer is to improve the code that is reviewed. Code reviews are not for training people. Training is for training people. And senior developers are not trainers. If you want someone to give training to your junior developers, hire a trainer. Not a senior software developer.

And if you don't want to hire a trainer, then tell your senior developers to give training to the juniors, don't abuse the code review process for this.

Maybe you just should fire this developer. He will find a new job in a better working environment, and everyone will be happier. Your only problem is that you are now short one senior developer. And the other senior developers may be more resistant to the frustration that you create for them, but eventually they will leave as well.

  • > Code reviews are not for training people < I disagree, it's a great way to train the whole team on how a feature works and what the app does – Matsemann Jan 18 '18 at 14:09
  • Most of us here agree "Don't abuse the code review process for this [basic training]", but it by no means necessarily follows "Either hire a trainer ($$$) or else force your senior developers to give routine training to the juniors". It's far better to lay down a process whereby the team generates an FAQ/ coding standards/ checklist/ wikis/ automated linting. Per my answer. 95+% of issues should then self-solve without needing a face-to-face meeting or training course. (But they have to be serious about instilling this culture, not just paying lip-service to it) – smci Sep 16 '18 at 22:14
2

Upon reflection, I think that the right answer here is to train the team on how to give and receive code reviews. I've expected people to understand them conceptually and to be able to implement them from my own explanations of the process.

Going forward, here is what I'll propose:
In our on-boarding of new developers, we inform them that we have a code review process and we even have a written policy on it that we have everyone read. We review that document annually and have everyone confirm that they've read the updated document afterwards.

If that written document proves insufficient, I'll ask for feedback from the rest of the team and possibly give a presentation on the topic.

A special thanks to @Lilienthal for pointing out that I might have overlooked the possibility that there are deficiencies in our existing processes, something I had dismissed outright but really shouldn't have.

  • Good on you for diagnosing and answering. Code reviews are susceptible to many antipatterns, like any other semi-formal communication where something's at stake. Always reframe someone's complaint into a more constructive *"How can we improve our code review process/ software process? Eliminate repetition? Gather and formalize our knowledge base/ codebase? Improve documentation?" – smci Jan 13 '18 at 19:52
2

Without code reviews, his colleagues will just change his code (without telling him), or they'll be too afraid to touch his side of the code base.

In an agile shop, neither of those options are desirable.

Code Reviews are for getting everyone up to speed on the same code base. They're a form of cross-functional decision-making and cross-functional training.

The potential disagreements and misunderstandings that code reviews bring up to the surface are not created by the code reviews.

Without code reviews, those disagreements and misunderstandings would just end up rearing their ugly heads at later dates (except the original developer who wrote the original code in question won't be there to explain his side).

That being said from the comments of your developer:

we do them individually from our desks. I always tell everyone I'm open to discussing things in more detail but we end up just trying to type a million lines of text into visual studio/tfs system – DefunctNinja

What DefunctNinja seems to be describing are not code reviews at all, and perhaps that's the problem.

For one thing, code reviews are supposed to be limited in scope and limited in time. And to me, typing "a million lines of text" sounds like those sessions are going on for way too long.

Also since those sessions are held so casually, it's doubtful that the other developers prepare for them by reading the code and annotating the reviewed code in advance either.

They probably just blurt out the first thing that comes to mind when reading the code over your video-conferencing session, hence their lack of filters and tact. And so that's not really a carefully crafted code review session at all, but more of an endless "bike shed" second-guessing discussion instead.

Parkinson shows how you can go in to the board of directors and get approval for building a multi-million or even billion dollar atomic power plant, but if you want to build a bike shed you will be tangled up in endless discussions.

Parkinson explains that this is because an atomic plant is so vast, so expensive and so complicated that people cannot grasp it, and rather than try, they fall back on the assumption that somebody else checked all the details before it got this far. Richard P. Feynmann gives a couple of interesting, and very much to the point, examples relating to Los Alamos in his books.

A bike shed on the other hand. Anyone can build one of those over a weekend, and still have time to watch the game on TV. So no matter how well prepared, no matter how reasonable you are with your proposal, somebody will seize the chance to show that he is doing his job, that he is paying attention, that he is here.

In Denmark we call it "setting your fingerprint". It is about personal pride and prestige, it is about being able to point somewhere and say "There! I did that." It is a strong trait in politicians, but present in most people given the chance. Just think about footsteps in wet cement.

source

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