What to do when the developer whose code I review becomes defensive?

Question

I am in the following situation. Small team within a large unit with many teams. There is an official code review process, but to a lot of developers it is more of a formality so it is a rare thing to see serious discussions or review.

Within my team I encountered a developer who was not following the routine and was committing straight to the develop branch. I opened the topic of code reviews, then we started doing pull requests. My pull requests were automaticaly approved. It was though obvious that he was not reading them, just pressing the button.

At the same time it appears he is taking it a bit personal when I am doing the reviews. I usually do quite thorough code reviews to be honest. I take it seriously. I have been doing code reviews to other developers in the company I work for it is just this one case where the developer appears to be extremely defensive. The problem is that we work very close.

Should I stop doing code reviews? How to approach this situation in the best way?

Just one fresh example to get the point. The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally. What am I supposed to do here? It appears the Tech Lead of the team does not care. He defends him a lot actually. He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind. Is avoidance the best strategy here, especially when the Tech Lead shows compassion to the guy?

Answer 1

I think that if your Tech lead definitely does not have your back, then you are going to have to modify your behavior.

This is the unfortunate reality of life in a company.

The best you can do is to ask your Tech lead to give you guidelines for what your code reviews should be like, and stay within those guidelines. Then, if the developer is getting his back up over something that you said in a review, then you can go to your Tech lead and ask him to back you up based on his guidelines.

Answer 2

I've been on both sides of this equation and while it's hard to say exactly what's happening with out knowing the people involved or how they see things, it does sound like you need to be little bit more tactful here. If I had to guess (and it is just a guess) your coworker is probably getting defensive because he feels attacked. To you it's an impersonal part of the job, to them you're some jerk who's getting mad at them for not doing it the way you think it should be done.

However I could be off base here. The first thing you should do is talk to them about it. This conversation does not need to be super formal but it should be private. Let them know that you're trying to help them improve and that your criticisms are not personal. Also don't just focus on the negatives. If they have any response to this then listen to them, and try to understand their point of view. However even if nothing comes of this conversation there are things you can do.

Does you company have any formal code standards? Are these standards documented? If so then your life just got a lot easier. Any time you see something that violates one of these standards politely point out the violation. If he gets angry you can even be like "hey I agree its a stupid standard but I don't make the rules". You can even make it look like you're watching his back by being like "hey dude, I don't want you to get in trouble, you should probably changes this so it matches the standard".

The other big thing you can do is explain why something should be changed. Don't just be like "hey change X to Y". Explain to him that X causes errors in realistic scenario Z or that Y is cleaner and makes the code easier to maintain. Again try to phrase this in such a way that your coworker feels like you're actively trying to help them improve instead of just reprimanding them because they did things their way instead of your way.

Point out the good things in their code as well as the bad. If they did something in an intelligent way then tell them you liked the way they did the thing. This will help prevent them from feeling like you're out to get them.

Answer 3

The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally.

A lot depends on how you commented on this. Compare:

This class looks like something in between Factory and Container, and it's named SomethingBuilder which isn't really related to the finally built object.

to:

I wonder if it might be worth the effort of splitting this in to a Factory and Container like so:

[.. small amount of pseudo-code to demonstrate what you mean ..]

I think this will give us [concrete advantage X].

What do you think?

Also, it seems to be that Something doesn't really covey the intent, as it's more of a Otherthing. Maybe rename it to Otherthing?

The first example, which is paraphrased from your question, amounts to little more than complaining about code and saying "your code sucks". I don't think it's particularly strange to take comments like that personally.

The second example offers an alternative, and also phrases it as a suggestion and asks for feedback, rather than just telling your coworker what to do.

He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind.

This just sounds dismissive of your coworker's views and conventions, and like you're not even willing to take it serious. In real-world programs things are often not all that neatly organized; there may be a good reason to write it like he did, but if your phrasing isn't communicating that you're open to alternatives then you're not going to have that conversation.

Maybe your coworker is just writing silly code; I don't know. But telling him "you're writing silly code" is not a very effective was of changing that. Ask why he did it the way he did. Listen. Provide meaningful alternatives and explain why you think it's better; give him space to contemplate it and experiment with it.

Don't expect him to change just by slinging the dictionary definitions of "Factory" or "Builder" at him; explain how it will make things better in objective terms. "More elegant" is often used, but not a very good argument. If you have difficulty coming up with better arguments than "more elegant" then maybe ... it doesn't really matter all that much?

I usually do quite thorough code reviews to be honest.

Some code reviews can be "too thorough", commenting on things that don't matter a whole lot.

In my own code reviews I always stop and think before leaving a comment: "will this really objectively improve the code quality, or is it just my personal preference?" Not infrequently, it's just my personal preference; so I let it go.

I've seen people comment on some very minor subjective things. It's okay to comment on a minor spelling error/typo in a comment, as that's objectively wrong, but I think it's important to acknowledge that programming is a very subjective craft, and that objectivity is hard to come by (this is why programmers are constantly fighting over everything).

Answer 4

It seems to me here that you are not the high man on the totem pole, as it were. As such, there's really only a limited number of things you can do, and it seems like you've done them:

1) If the team isn't doing PR/CR process, you should push to institute it (you did).

2) If people are rubber-stamping PRs, you should urge them to stop (you did).

3) You should be the example and when someone sends you a bad PR you should comment all over it and explain what's wrong (you did).

4) If people are not working up to snuff, you should bring this to the attention of the tech lead (you did).

At this point, it's the tech lead's responsibility to handle the situation as they want. In this case, the culture comes from the top; the most experienced developer needs to either explain to the rest of the team why the PR process is important, or impose that they have to do it properly even if they don't understand why (optimally the former, but the latter works too). However, if the tech lead doesn't understand why it's a valuable process or doesn't want to put in the effort to make sure it's done properly, there's not much you can do.

As for what you should do going forward, much as it pains me to say so, you should bring up issues, but if the people on the receiving end of the issue think it's a non-issue, then you should rubber-stamp them. You're in danger of going from "the responsible one" to "the anal-retentive one", and you don't want the reputation of being hard to work with, it's not worth it. I would also recommend searching for a new position; you don't want to work with bad developers who are going to bring you down with them. Let them stay on their sinking ship, and you should take your responsible approach to development to a company where such a skill is valued.

Answer 5

I would say stick to guns here. Your code review sounds reasonable. If your tech lead is allowing poor naming conventions it will cause confusion among developers that did not write this class. Nameing conventions are useful for many things and standard naming convention are the way to go.

Answer 6

I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.

Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.

If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.

As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.

It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?

If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.

A Discussion on Naming

Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.

Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.

Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:

1. describe the output
2. describe the business logic contained within

Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:

saveDataInAthreadSafeAndDataConsistentManner(){...}

isn't useful. But

saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()

well, that's a useful method name.

But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.

Another Thought

Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.

Answer 7

Here's a few practical ideas for resolving this.

1. Try to separate code review comments into objective and subjective points. Objective points include observations such as "there is a potential bug here", or "this does not match the requirements". Subjective comments include "there's a better way to do this", or "this would be better named X". It' often helpful to be explicit in comments about the difference: "In my opinion..." / "Consider trying...". I think this helps reduce defensiveness.

2. Accept that, unless you are in a more senior position, you can only suggest improvements where points are subjective. You have to accept that there will be some differences in style in the team, no matter how much it seems obvious to you that one way is preferable.

3. If discussion seems to be heated, it's often helpful to chat in person rather than through whatever PR tool you are using. You can have a more in-depth discussion, and be more tactful with appropriate body language. You might still disagree, but it might be easier to see each other's point of view.

4. For things like naming conventions, it's often useful to get team agreement on things like what to call modules of type X. A Slack or other internal communication tool often offers a poll option for team members to vote on preferred naming conventions. Once you have team agreement you can point to that in future code reviews. Offer to set up a wiki page with your team's decisions.

5. With respect to higher level questions about PRs getting nodded through, or how to resolve conflict over PRs, these are good topics to bring up in a retrospective (assuming your team follows some sort of Agile process).

Answer 8

Your coworker seem to disaprove the entire review process. Your tech lead is skeptical. This is both of these issues you should adress, beginning with the one that have the most to say about the process.

The tech lead

The tech lead may be skeptical by your way to do reviews or with reviews at all. You need to find out why is he defending your coworker. Be open to criticism. Maybe you are too formal. Maybe the way you give feedback doesn't seem gentle enough.

Then you should sell him that reviews are however necessary to ensure code quality, and team level. Suggest that the team may grow in the future and these issues need to be adressed when it's less painful.

His firm approval is key to the success of reviews. If he doesn't agree, you can simply let go with the review process, but that doesn't mean you don't have an issue with the coworker.

The coworker

Developers in general (me definitely included) tend to put a lot of ego into their work. The problem with reviews is not new, some people do take it personnaly as criticism of their work. This is critical, because developers are expensive and good morale is important.

This is why, no matter if your tech lead approves reviews, you should stress on the fact that despite reviews were hard, you do think he is doing good work, and he shouldn't feel insecure. Check if he isn't coding in a rush as well, remind him if necessary your preference for quality over quantity.

Answer 9

Sad to say but feel you need to leave that team, maybe leave that company. You are not a good cultural fit. Both your team leader and whoever promoted your team leader have chosen to reward a particular pattern of behavior. The chances of both those people leaving and the whole culture changing are very small.

Instead of rewarding you for caring, your team leader has been disrespectful and dismissive of your attempts to improve the quality of workmanship within your team. This puts you in a politically weak position and easy to blame for slowing things down, when in fact the code is ”written once and read many times”, easy to read and understand and maintain means a quicker path to bug free product. From your own description your colleague's code style slowed you down by 30 minutes to work out exactly what just one class did. Surely such a description should have been part of the class' name.

In addition your management chain has ignored basic math and basic psychology.

For the math, if one module has a 99% chance of no bug (I e not using sufficient unit tests), then you can calculate (1-p)^n that 100 modules together have a chance of 37% of bug free behavior. The probabilities go against you very quickly. Improving the ”quality” to 99.5% improves the system to just over 60%.

For the psychology, who wants to work for or with people who don't care?