managementCode reviews (a.k.a. peer reviews) must be a mandatory practice for every serious software development team. I hope there is no debate about this. Some do pre-merge code reviews, protecting their master/development branch from accidental mistakes. Others do post-merge regular reviews to discover bugs and inconsistencies after they are introduced by their authors. Some even do both, reviewing before merges and regularly after. Code reviews are very similar to a white-box testing technique where a tester looks for defects with full access to the sources of the software. In either case, a code review is a great instrument to increase quality and boost team motivation.
However, it’s not so simple to do them right. I would even say it’s very easy and comfortable to do them wrong. Most code reviews and reviewers I’ve seen make similar mistakes. That’s why I decided to summarize the four basic principles of a good reviewer as I see them. Hopefully you find them helpful.
No Fear
There are a few different fears a serious code reviewer should renounce. The first and most popular is the fear of offending an author of the code. “I’d better close my eyes and pretend I didn’t see her bugs today so tomorrow she will ignore my mistakes”—This is the kind of attitude this fear produces. Needless to say, it’s counterproductive and degrades code quality and team morale. Here is my advice: be direct, honest, and straight-forward. If you don’t like the code, you don’t like it. You shouldn’t care how your opinion will be taken by the author of the code.
If you do have such feelings toward your colleagues, there is something wrong with the management model. You’re afraid of being rejected by the team for “not being a team player,” which is a label attached to you by the weakest members of the team, not by the project sponsor. The sponsor pays you to produce high-quality software. The sponsor doesn’t care how much your intention to increase quality offends others, who care less. The sponsor wants his money to produce deliverables that can be sold to customers and returned back in profit. The sponsor is not paying you to make friends in the office.
The next type of fear sounds like this: “If I reject this code, I will delay the release” Again, it goes without saying that such an attitude does a significant disservice to the entire project. You will accept the code and close your eyes to what you don’t like in it. The code will go into the next release, and we’ll ship it sooner. You won’t be a bottleneck, and nobody will say that because of that dogmatic code reviewer, we delayed the release and lost some cash. You will be a good team player, right? Wrong!
As I’ve mentioned before, a professional team player understands his or her personal role in a project and doesn’t cover anyone’s ass. If the rejection of bad code delays the release, that’s the fault of its author. Your professional responsibility is to make this fault visible. That’s how you help the team learn and improve.
I think it’s obvious that the education and improvement of a team first requires getting rid of bad programmers and promoting good ones. Honest and fearless code reviewers help the team learn and improve.
Yet another fear is expressed like this: “I may be wrong and they will laugh me out” Even worse, they may spot my lack of knowledge. They may see that I don’t know what I’m doing. It would be better to stay quiet and pretend there are no bugs in the code. At least then I wouldn’t embarrass myself with stupid comments. You know that it’s much easier to look smart if you keep your mouth shut, right? Wrong!
The project is not paying you to look good. You’re getting your paychecks not because the team loves you but because you produce deliverables on a daily basis. Your professional responsibility is to do what’s best for the project and ignore everyone’s opinions, including the opinion of your boss. Similar to A Happy Boss Is a False Objective, I would say that the respect of the team is a false goal. Don’t try to earn respect. Instead, create clean code and respect will come automatically.
Let me reiterate: Don’t be afraid to embarrass yourself by making incorrect and stupid comments about someone’s code. Be loyal to the project, not to the expectations of people around you. They expect you to be smart and bright, but the project expects you to say what you think about the code. So screw their opinions; do the right thing and say what you really think.
No Compromise
Okay, you’ve fearlessly said what you thought about the code and simply rejected it. The branch you were reviewing is not good, and you explained why. You asked its author to refactor here and re-write there. What’s next?
He or she will try to make a deal with you. It’s natural and it’s happening in almost every branch I’m seeing in our teams. The author of the code is also a professional developer, and he also has no fear. So he insists that his implementation approach is right and your ideas are wrong. What should a professional code reviewer do in this case?
The worst thing (as in any conflict resolution) is a compromise. This is what ruins quality faster than bad code. A compromise is a conflict resolution technique for which both parties agree not to get what they wanted just for the sake of ceasing the conflict. In other words, “Let’s make peace just to stop fighting” It’s the worst approach ever.
Instead of a lousy compromise, there are three professional exits from a fight over a piece of code:
“You’re right; I take my comments back!” This may happen, and it should happen very often. You should be ready to admit your mistakes. You didn’t like the code, but its author explained to you its benefits, and you accepted the logic—not because you want to stop fighting with him but because you really understood the logic and accepted it. Willingness to say, “I’m wrong,” is the first sign of a mature and serious developer.
“I will never accept this, period!” Some code deserves that, and there is nothing wrong with resolving a conflict this way. The opponent may accept the situation and re-write everything. And learn something too.
“Let’s do what the architect says!” In every project, there is a software architect who makes final decisions. Appeal to his opinion and get his final decision. Invite him into the discussion, and ask him to take one side in the conflict. Once he tells you that you’re wrong, accept the decision and try to learn from it.
Thus, either stand strong on your position and fight for it or admit that you’re wrong. One way or the other. But don’t make a compromise!
Don’t get me wrong; it’s not about being stubborn and holding your cards no matter how bad they are. Be flexible and learn on the spot. Your position may and should change during the negotiation, but don’t accept anything that you don’t like. You can exit the conflict either by being fully convinced that the opponent is right or when the architect says so. Nothing in between.
No Bullshit
Again, you fearlessly said that a method should be designed differently. Your opponent, the author of the code, replies that he doesn’t think so. You take a look again and decide to stand behind your position. You still think you’re right, and you’re not going to make a compromise. Now what? It’s too early to call an architect, so try to convince your opponent.
In most cases, convincing is teaching. You know something that he doesn’t know. That’s why he created that method the way you don’t like. One of you needs some additional education. Here is an opportunity for you to be a teacher of your colleague. To be an effective teacher, you need to show proof. You need to ground your logic and make sure he understands and accepts it.
Be ready to show links, articles, books, reports, examples, etc. Just “I know this because I’ve been writing Java for 15 years” is not enough. Moreover, this type of argument only makes you less convincing.
If you don’t have enough convincing proof, think again—maybe you are wrong.
Also, remember that it’s your job to prove that the code you’re reviewing is bad. The author of the code should not prove anything. His code is great by default. The job of the reviewer is to show why and how that’s actually not the case. In other words, you’re the plaintiff and he is the defender. Not the other way around.
No Offense
This is the last and most difficult principle to follow. No matter how bad the code is and how stubborn your opponent is, you must remain professional. To be honest, I find this very difficult sometimes. At Zerocracy, we’re working in distributed teams and hire a few new people every week. Some of them, despite our screening criteria, appear to be rather stupid difficult to deal with.
I encountered a funny situation about a year ago when a new guy was supposed to create a small (20 to 30 lines of code) new feature in an existing Java library. He sent me a pull request (I was a code reviewer) after he put in a few hundred lines of code. That code was absolute garbage and obviously not written by him. I immediately understood that he found it somewhere and copied it. But what could I do? How could I reject it without saying his attitude was unacceptable for a professional developer? I had to spend some time objectively blaming his code for its style, its design, etc. I had to make many serious comments in order to show him that to fix it all, he should delete the garbage and re-write it from scratch. I never saw him again after that task.
My point is that it’s easy to be professional when you’re dealing with professionals. Unfortunately, that’s not always the case. But no matter how bad the code in front of you is, be patient and convincing.
What do you feel when your pull request is rejected and a lot of "re-work" is required? #sociotech
--- Yegor Bugayenko (@yegor256) July 1, 2018
