What is the best way for a programmer to handle being code reviewed?

Question

I am fairly new to code review, but I have been coding for years during my PhD - which doesn't always make you a good programmer!

If the reviewer changes your code and goes through it with you line-by-line, what do you do if you disagree? Sometimes you made choice X and the reviewer changes it to Y, and you had Y in your mind but decided not to do it because of the disadvantages, but the reviewer asserts that those disadvantages are not important. Should you verbalise your disagreement, or just not and listen to them?

Its difficult if you are not good at accepting criticism, and if the reviewer is a more senior person in the organisation. It would not be good to come across as too defensive.

Answer 1

True, coming across as defensive is not helpful; but then - neither is being criticised, so if you feel this is happening you really should vocalise your concerns that the code review is not helping you write better code within the organisation.

The reviewer has a responsibility to review code for real non-compliance and defects, not use it as a means to write your code the way they would have done. This means the review is there to review how you did something, and point out any areas that you made a mistake (something the best of us do) or didn't understand the framework or standards or "historical reasons" some code is written the way it is where you are.

So if there are multiple ways of doing something, there needs to be a good reason why your code is changed to an alternative way, otherwise its simply non-constructive churn that won't help you.

Also, remember that the reviewer isn't perfect either. They may have an idea that Y is the way to do it, and haven't realised that X is better. You should explain the reasons why you did it in way X. The reviewer might agree with you, or he might tell you why Y is a better solution - there may be other factors that you don't know that he does.

In short, reviews are a way to get team members to communicate about their code changes. So talk to the reviewer about it.

IF it helps, talk in an impartial way, as if you were not even the author of the code being reviewed. The code belongs to the company or team after all, and the team will have to maintain it. You just happened to be the guy who wrote it, that's not as important a factor as many believe.

Answer 2

Writing computer code is a prime example of making decisions under uncertainty. There are always conflicting pressures, and how you decide in any given question depends on what pressures you perceive and how big you consider them.

Therefore, when a reviewer disagrees with your decision, that means they see different pressures/risks than you do, or they consider some of them larger/smaller than you do. You must absolutely talk about these differences, because not doing so perpetuates the problems that led to disagreement in the first place.

If your reviewer is more senior, their experience may correctly tell them that this or that risk is not very relevant in practice, or they may know that some kind of error has a long history of occurring in your organisation, and it's worth being extra careful to avoid it. Conversely, if you feel that you know something your reviewer probably doesn't, you must absolutely express that; keeping silent amounts to a dereliction of duty on your part.

The most important part of handling the situation is to understand that criticism of design decisions is virtually always not a criticism of someone's personality. (In the rare cases where it actually is, you'll notice that soon enough, and if you truly cannot please somebody, nothing you do makes any difference, so where's the harm in following best practices? Far better to find a better position as soon as possible.) It is just a result of different people having different perceptions of the many risks and rewards involved in computer code, and given how complex modern computer code is, that is only to be expected. Talking about the differences helps improving the code and improving your cooperation in this case and in future cases.

Answer 3

The other answers contain very good information already. I would like to expand a bit on some aspects that have been hinted at by gbjbaanb (see my comment to his answer).

In my experience I have observed different kinds of feedback during code reviews:

1. "I honestly believe that this is wrong / suboptimal and that you should change it this way." I normally take this kind of feedback seriously, and I will only oppose the suggested change if I think I have a strong point against it.
2. "I find your solution OK, consider this alternative but it is up to you whether you accept the change or not." I take this kind of feedback seriously too: the reviewer is suggesting an alternative, even though they do not have a strong opinion that their solution is better. I have an opportunity to learn something and I will accept the change if I like it better. Otherwise, the reviewer finds it OK if I keep the code according to my taste.
3. "I would have done it differently so you have to change it, period.", or even "Oh, I have changed your code because ...", the change has not been suggested, it has been committed already! Some quick explanation regarding the change is given, with the hint that there is not much time to discuss the details because we have to move on to the next task.
4. The reviewer suggests small changes of trivial nature that do not improve the code but just make it different. When asked to discuss the suggested changes, the reviewer gets into lengthy discussions about trivial details until you give up out of exhaustion.

Options 3, 4 may come disguised as 1 and 2: "It was really important to do it the way I have suggested." or "You can refuse the change if you really want.", said with a tone that in fact means just the opposite of what is being said. In case you oppose to changes you consider unnecessary, shared code ownership can be used a justification of this attitude: "The code does not belong to you, it belongs to the team!" You can tell when the reviewer's intention is not honest if the reviewer is not very open for discussion, gets irritated and tries to push their solution at all costs. Basically they have already decided, and any further discussion is just a waste of time.

IMO options 1 and 2 are a sign of an honest reviewer who is trying to help, is trying to teach you something while respecting your work, and is also open to learn something himself. In this scenario I try not to be too proud when I get constructive feedback from a reviewer.

Options 3, 4 suggest that there is some power game going on: what is important is whether we do it my way or your way, not that we try to find a good solution that satisfies both. This may be related to the reviewer's ego, but also to them being unable to understand any code that does not follow their way of thinking. They are normally disturbed by code that does not look familiar to them and would like to impose their way on the whole code base.

If situations 3 and 4 happen too often, team work can get quite unpleasant. In such a situation I would consider leaving the team.

Answer 4

To address your issue of what to do when you disagree...

Talking it our is the way to go as most folks have already pointed out.

If you have already done that, maybe more than once even, one useful technique we use is if there is still disagreement in some area(s) is to say 'yeah, it would be good to refactor that -

Can we put in a separate ticket for that?

A separate ticket allow you to get the code in, instead of the dreaded 'churn and never move on' mode that I've known well at some places. This fits in well with agile, doing the smallest amount 'possible' and spreading the load. Sometimes yagni will end up applying. Some times the product manager will decide that there are more pressing needs than that refactor in terms of value to the client, deadlines and resources.

Answer 5

Code review is probably a good thing in general, but from my experience it is best served as a tool for developers on new teams using new coding guidelines or for fixing important bugs, so that the risk of doing errors gets reduced. If you believe that you know better than the reviewer, you should ask why the solution he or she proposes is better and argument with your insights on the matter. Nobody knows everything best, so code review should or could be a valuable learning experience for both.

Answer 6

The code review is both an opportunity to catch potential issues and to pass on knowledge, for both reviewer and coder.

As a code reviewer the responsibility is to highlight possible areas of risk, non-conformance to standard practice, improvements, and generally just another point of view on the same area of code.

This should not result in a change to code without discussing/understanding the coders decisions at time of development.

If the reviewer is making changes they may have difficulty delegating work to others, which is tough for a lot of smart people to do.

As a coder receiving a review you are being protected from a possible issue when deployed, being given the chance to learn something new, and the opportunity to share your knowledge with the reviewer.

Irrespective of seniority your way of thinking can come up with a solution that just doesn't occur to some, so the review can be your chance to shine just by doing what you believe is right.

As long as both coder and reviewer accept that they can be wrong, and that it's ok to be wrong, a review becomes something that strengthens the team because you work together on the solution.

Answer 7

It looks like you haven't had your code reviewed yet :-)

The goal of the code review is to get code in decent quality, and to know that you have got code of decent quality. When the code of an inexperienced developer is reviewed, then it can be used to teach how to write better code, while avoiding to frustrate that developer.

The reviewer should never change your code. They can make more or less strong suggestions how they would like your code to be changed, and they can decide whether to accept your code or not.

If the review goes right / if I review your code, what you will likely get is some comments how I would write the code which you can learn from, or ignore - these are things where I have an opinion and you are free to have a different opinion. In my area, good naming of functions, variables and so on is considered important, so you may get some suggestions to improve naming. Usually you should make changes in that case (sometimes by finding an even better name for something). Sometimes I'll find bugs. You fix them. Sometimes I'll find things that I thing are bugs, and I'm wrong. If it's hard to see that the code is correct, you make it more obvious correct. If I just got it wrong, you tell me.

If I think that the design is generally not right, then this should have been discussed earlier. We should then think about whether your design is good enough, taking into consideration how much work is involved in a change, or maybe I was just wrong and your design is better. We should end up with agreement.

If reviewer and reviewee cannot agree, then we have a problem. Because it means that one of us is incapable of teamwork, or one of us is uncapable of distinguishing between a good or bad design, or both. This is not necessarily your fault. Unfortunately there are developers who are senior and clueless, and that will be a problem for the company and for you.

If it happens, think very, very hard: Do you have a problem accepting well-founded criticism? If that's the case, you need to change your attitude. Are you too inexperienced to see why the reviewer is right? If that's the case, that's no problem. Trust the reviewer and learn. Are you sure that you know better than the reviewer? Accept the review, but ask a third, trusted developer about his opinion. Remember you can be really sure of yourself and be right, but you can also be really sure of yourself and be wrong.