3

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.

  • Hello opel, and welcome to the site. I edited your question to make it easier for us to read. If you don't like the edits you can edit it again. – MackM May 25 '18 at 13:43

11 Answers 11

10

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.

  • 1
    I will often move complex conditionals out into their own function if that's not going to cause too much parameter passing. I find it much clearer and I figure the compiler is just going to inline it anyway so there's no performance hit. I try to avoid going more than one block deep in a function. – Loren Pechtel Oct 10 '15 at 2:06
  • 1
    I asked why. He did not tell anything. After I showed example that others which I assume are good are doing this - just got reply that I wanted to show him that I am right. So it means he did not have real arguments. @LorenPechtel - if there are lot of parameters - I am ofen puting those as private class variables and then every function see them in the class. – opel Oct 10 '15 at 7:24
  • Not to mention more thorough code coverage in automated testing! – Wesley Long May 24 '18 at 22:35
  • It also makes unit testing easier and the code more legible, hence maintainable. – Mawg May 25 '18 at 9:55
9

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.

  • If its his opinion, then he should not make it looking like its a rule I assume. He can say that it is his opinion, I say in my opinion its difrerent, then ok, we can have different opinions. But it looked not like opinion, but like a way to show everyone how bad I am and how good he is that he spotted this. – opel Oct 9 '15 at 18:54
  • 2
    There are always going to be people who get their satisfaction from believing that they are RIGHT, regardless of if it's actually of value or not. Learn to discern these people and when you get in a conversation, just nod your head a lot because it becomes useless to disagree. Find some excuse to get out of the conversation ASAP!!! – Xavier J Oct 9 '15 at 18:58
5

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.

  • "My first advice is to take it easy. " - this I try to do often and get bad emotions like anger. – opel Oct 9 '15 at 18:47
  • But when he tells me that creating function for one time use is bad - I bet that its not what everyone thinks. – opel Oct 9 '15 at 18:52
3

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?

  • Its not about code practice, but its in general about blaming for something when he should not. Code practices was just an example for ilustration – opel Oct 9 '15 at 18:39
  • Then it was bad example of blaming. Learn to take criticism and use less confrontational language. – paparazzo Oct 9 '15 at 18:45
  • 1
    thats easy to say :) I mean I need to learn to not get bad emotions, but how to do that ?:) With bad emotions, I can be silent and not using confrontational language. – opel Oct 9 '15 at 18:57
2

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.

1

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

  • Ok, lets say I am the good guy who invites him politely to a meeting to talk about the problem. Still there is high chance that he will tell same arguments - that I was wrong in the situation and he was right, so he has the right to blame me. – opel Oct 9 '15 at 18:44
1

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!

  • "If no, go ahead and follow the recommendation. " - I ask my boss, and if he also tells to do same, then I do what my boss says. – opel Oct 10 '15 at 10:26
  • Yes, I cannot eliminate bad critisims. But we can find out if it is bad or valid by discussing and finding a facts on google from valid sources. And then he should admit that his criticism was bad, like I admit my mistakes when his criticism is good. That is what I expect from smart, respected people. – opel Oct 10 '15 at 10:28
  • Totally disagree. Changes are work and introduce bugs. So the question should be: Would it have a significant positive impact to the code base if I followed the recommendation? – gnasher729 Oct 10 '15 at 22:20
  • @gnasher729: The assumption here is the other guy strongly thinks it should be followed & it doesn't come with significant negatives, like lots of work. E.g. if someone strongly feels x should be a separate function, why not do so even if you don't really think it matters; at least you can move on and do something useful. – user9641 Oct 11 '15 at 6:16
1

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.

0

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.

  • A function that is used only once may be used again tomorrow. It may be already planned that it will be used again. – gnasher729 May 24 '18 at 23:46
  • @gnasher729 Are you advocating not writing unit tests? That seems like an odd thing to advocate. If it's used once, there's no unit test. – Edwin Buck May 25 '18 at 14:25
-1

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.

-1

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.

protected by Chris E May 25 '18 at 21:16

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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