How to deal with coworker giving unhelpful criticism during code review?

Question

I am struggling with a coworker who provides heavy criticism during code reviews, to the point of turning them into arguments about "who knows more" instead of keeping them focused and helpful.

Sometimes he provides useful feedback. For example, he pointed out that when I write my code comments in English, they are hard to understand - instead, he tells me to write them in my native language. Ok, I agree with that.

But, he also once told me that it is bad to create a new function when I use it only once - I did not agree with that, and I felt like he was just trying to start an argument. I provided him with an example where a high profile, widely respected developer had done that, and his response was "you read any guy on the internet and believe what he tells?" I said no. He also said, "you just found that to show you are right."

Whenever he is reviewing my code, and I do not have a good argument - it is a bad situation, because he says, "that's not a good argument." Yet, when I find a reasonable argument, he still attacks me and tries to disprove me. My question is, what can I do to help us have a helpful code review instead of turning it into an argument? When it turns into an argument, I feel badly. I'd like to get good feedback, but ultimately, I don't want to feel bad afterwards.

Some solutions I have considered are,

Should I not respond to him at all? I do that often. But when I do that, I feel anger and other bad emotions. It happens when somebody tells me something bad about me without a good argument proving to me that this is real and not just their thought.

Should I just change my job?. But I believe that in almost every good job, there will be people who try to show you that you are bad when actually, you are not bad at your job, at least in that certain situation.

Should I ask him not to insult me? But that just makes me look weak, like "poor boy here is not able to take revenge so he just asks us to not attack him."

The whole situation reminds me of a game. For example, in a game we fight against each other, and winning is sweat. And in a game we are not asking politely - do not do this or that, because I will hardly win then. I feel like, in a code review, we should be polite, instead of treating it like a game that we're trying to win at all costs.

Instead, he is turning the code review into a game, but the game is not a computer game or a sports game - it is an argument game, a knowledge game. Who is better at code. Imo.

Answer 1

Making something a function is not only for reuse but also to reduce complexity. Moving 6 lines of code out of a 20-line block is good, moving two lines out proly not, so it depends what you did.

You need to discuss the whys, not the whats. Ask all the time why, might help you understand how others (later maintainers) see your code.

And insist on politeness if he is rude, things can always be discussed polite.

Answer 2

Peer review is opinion-based. He's not your boss. You can take the information constructively, or you can ignore it altogether. If he's pointing out things that are objective (i.e. ways to measurably improve performance, maintainability, reliability, scalability) work with those, but with the other stuff you can't let it bother you so much. Someone's always going to have an opinion different from yours.

Answer 3

Peer review is a fairly common practice in the industry. It sounds to me like you take it personally when someone says something about your code as evident by the fact that you want to "show them" what is wrong with their thinking/code.

My first advice is to take it easy. My advice is to try to prioritize what they are saying from "critical" to perhaps "nice to have."

On the flip side, sometimes I think companies have inconsistent structure when it comes to code review/peer review. First and foremost is a "best practice" guideline should be established when it comes to coding. Things from naming conventions, to how databases integrates into the code should be established. It doesn't have to be based on any industry-wide best practices but should be based on it loosely and fitted into your particular industry.

Some places have unwritten best practices but I think most places sort of lack. What I would do is first ask the person to prioritize his comments from "critical, must have" to "nice to have" categories. If EVERYTHING is critical, must do, and you have no established best practice, then I would go to the boss and explain that there needs to be best practices since one person's code review is very loose while another is hyper-critical wanting to change everything but following no set guideline.

Remember the overall goal of peer review is to have someone look over your code to see if you miss anything business-logic wise, security-wise, or general easy to read code. I suggest picking up some coding books and see if anything this person is saying matches to what is best known in the industry. If what he is saying is widely used, and others in your company agrees, then I would see this person as a mentor rather than someone "out to get you."

Edit: If you think this co-worker is bad, try submitting some open-source code on github. It's a sure bet your code, 95% of the time will be shot down. You can't let emotion drive you. Instead you have to look at it from the standpoint of the general consensus and what everyone thinks is the best. There are times when I work on a code for a solid week and it get peer reviewed, and we ultimately decide to not do it and I simply save it. Sometimes it comes back, sometimes it goes dead. Just don't take it personally.

Answer 4

Blaming is a bad title. He is not blaming you for a program crashing that was not you fault. He is disagreeing with some code practices. There some code practices that are clearly bad (repeated code blocks) and lot in the middle that people are not going to all agree on. Try and turn it into acceptable code practices rather than best. Is a single use function acceptable - I hope yes. Try and get it moved from optimal code to acceptable. Does it perform? Can it be maintained?

Answer 5

Part of the office-life, especially for programmers, is learning to sometimes deal with somewhat challenging personalities. They may insult you, they may hurt your feelings.

Sometimes, you try to extract the good parts and leave the bad parts. I had a former boss you used foul language frequently. It would bug me, but I bit my upper lip.

If he says "This line of code looks like @#$@*& and I'm seriously @#$@#$@#" then I would nod my head and try to be patient.

But one day we had a review meeting, and he reviewed my performance. Then at the endof the meeting, he asked me "Is there any suggestion you have for me?" And I said ", Well, I feel that you can tone the language down a little"

Afterward, I noticed a really strong decrease in the bad langugae. So I think, the best time to make advice may be around performance review. But if it is a serious issue, you can always politely communicate. Ask for a short meeting, Face to face is better than email. But be polite and professional

Answer 6

Code reviews are mainly done for two reasons: To stop bad design and bugs to enter a code base, and to educate less experienced developers. It seems the latter is not the case here, so the idea behind the code review is to stop bad design and bugs.

In a good environment, if the reviewer finds bad design or bugs, that is appreciated and gladly fixed. There are situations where it is opinion based or style based which way to write code is the best; in that case it is absolutely fine for a reviewer to point out a way that is better, or that the reviewer thinks is better, but that isn't so much and better or so undisputed better that it is worth investing the work for a change.

Your environment seems to be toxic. Code reviews seem to have deteriorated to power fights. So your coworker demands that you remove all small functions that are called only once. Why don't you demand that he extracts code into small functions when you review his code? The outcome, if you both do as you are told, is ridiculous: You change your code to something that you feel is worse, and he changes his code to something that he feels is worse. Totally ridiculous.

What actually needs to happen is that your manager bangs your heads together and tells you how to do code reviews. The reviewer can state objections to the code, and the objections must be answered. That doesn't mean the reviewer can act as a tyrant whose orders need to be obeyed. If I told you "this function should not be a separate function but incorporated in other code" then stating "being a separate function makes this more adaptable and maintainable, and even if it where true then the benefit would be so small that it doesn't justify the effort to make the change and verify that it is correct" is a perfectly reasonable answer.

Answer 7

Code reviews and code standards are a means to an end. When done correctly, the goal is not to meet a standard, or find any possible improvement in a review. Rather, the goal is to write software that is suitable for a given use case, and will be sustainable/supportable.

To put it a different way, your goal is not "perfect" code. Your goal is "suitable" code. Anything more is essentially a waste of resources.

With that in mind, your question was,

So what else can I do here?

First, you need to filter your coworker's feedback: Is it helpful in getting your code to the desired standard?

1. If Yes: Incorporate the feedback and move on. It's easy to feel like he's attacking you personally. You need to be able to separate personal emotions from your work. We all will have gaps in our work product some times, this is why we do reviews.
2. If No: Let him say his piece, and do not give an emotional or argumentative response. If you respond, you're playing into his game (the game you acknowledged in your update to your question). It takes two people to play a game. If you stop playing, the game will stop!

If there are no documented standards in your environment: It'll be harder to make the decision between the first and second case, so you'll just have to be your own judge. But, the main trick is, don't play his game. Instead, turn this around, and focus on filtering out the helpful feedback, which you can incorporate, and learning to ignore the unhelpful feedback.

No matter where you work or what job you have, there will always be people who want nothing but to point out how you're not good enough, or they're more skilled than you. One of the most fundamental life skills you can focus on - both for your own personal satisfaction and to ensure you're improving as an employee - is to learn to filter out things that matter from things that don't.

Answer 8

it is an argument game, knowledge game. Who is better at code. Imo.

I think this is your biggest problem. It's not a game, it's a collaborative process to create something better. You need to learn to stop thinking in terms of winning and losing. I understand why you feel the way you do because it's human nature. But with experience and nurturing confidence, you can overcome this and start to appreciate the occasion to reflect on your views.

There would be a lot of criticisms that aren't valid and you disagree with. You can't really eliminate them. Just like nobody can write bug free code all the time, nobody can make perfect judgements all the time. You need to cut a slack, not least because you are doing it, too.

When it happens, you should ask would it have a significant negative impact to the code base if I followed the recommendation I disagree with?. If no, go ahead and follow the recommendation. It makes people feel good, so at least you are getting value there. If yes, weigh the two options very carefully again; sometimes it's not only about technology, it's also about people. Truly good developers can work with code AND people. If after weighing-in the human factor and you still think the impact will be net-negative, that's when you make a case against. However, make sure that you ALWAYS try to find out the reason for the recommendation first! In lots of cases, you just haven't noticed that point of view yet. And that's when you grow as a developer.

Forget about pride, it's way overvalued!

Answer 9

Solve the problems by addressing them.

• Your function is only used once.

Honestly, a function that is only used once can easily be inlined. Perhaps there is a good reason not to inline it. If there is, then leverage that reason.

I don't know what that reason might be, but I do know that if the function is really only used once, then it's not used by testing. That means you don't have a unit test on the function. Write one, and then you have better code quality (not necessarily a better solution, but at least proof that the solution does what was intended). You also have two places that use the function.

Trying to find ways where both sides can get a bit of what they want can feel like a chore; but, in the long run you will be more effective.

Answer 10

I think I understand, I had a friend who was in the same situation. His team mates didn't like him for personality reasons, and were extremely critical of his code, while overlooking much bigger faults to each other. They held his code to high standards that they essentially made up, so he couldn't possibly get them right. In the end he just showed everyone the middle finger and left.

Maybe your your manager can scold them, since they are holding up production by bloating review feedback uselessly, but in my friend's case he didn't care. If that doesn't help and you want to continue working there, my advice is to try to focus on understanding why they don't like you, and change that. Otherwise, you'll have to learn to control your emotion and endure the criticism even if unfair.

Answer 11

My recommendation is that you read the book The Gentle Art of Verbal Self-Defense by Suzette Hayden-Elgin. It will help you deal with people who are less than professional in their dealings with you.