By Dominik Malik, Frontend Developer II
Every time I go to my own Merge Requests (MR) list, I have one thought in my mind, “I hope it's been approved already!”
And when I realize there’s no approval from the other developers, a second thought appears, “They’ve forgotten about it!”. But that's OK. If it’s an easy sprint, I can wait. In the meantime, I can take care of tasks that do not share code with the MR, so I won't have a problem with rebasing my code. But this is a really happy scenario, and when you’re working in a team, it is rarely the case. So I start to get nervous, impatient, and remind them that I really need this approval, right now. The reviewer says, "Ok, give me a moment". I refresh the page and… WHAT’S THIS?! - "15 UNRESOLVED THREADS”
Have you ever had such an experience?
Most likely, if you’re anything like I was, your first thoughts here will contain the foulest language possible. This is the perfect moment to stop and think things through. That’s because the reviewer was only doing their job. The reason they added those comments was actually YOU! In this article, I want to show you that you should be grateful for your Code Reviewer, and that they are not, in fact, your opponent.
Though you can still have a good old dispute with them.
What makes a good code review?
There’s as many points of view on this as there are people doing the reviewing; however, in my opinion, the following are key and should be consistently checked:
Code standards in the company
Recommended solutions from the library documentation
The Don’t Repeat Yourself (DRY) rule
The complexity of the code (lines, functions, classes, files, etc.)
The style guide
Test coverage
Code readability
Whether the functionality of the added/changed code is correct
This list could be much longer, and even then it would not satisfy everyone. But there are also other things to consider, that we will discuss in the next part of the article.
Getting back to the point of when we saw the unresolved issues, if you are the kind of person who just starts correcting the code and applying the suggested changes to the MR, then simply skip to the next section. However, if you’re not, then maybe the hints provided here will help you get through it all without problems. The most important thing is to not blame anybody or yourself for this issue. It's normal for someone to notice something in the development process that you could improve or might have forgotten. KEEP CALM and just read the notes.
When creating the code, you always do your best (I hope) and it’s unlikely that you missed something, forgot about a corner case, or skipped one of the points from the list of good code practices. While it's not a bad thing, the process of code review is designed to avoid problems generated by a person's mistakes. In many companies, there are even two approvals required to unblock the MERGE button. I think that’s great. Your GitLab admin can impose these rules and, believe me, you will profit from it.
“I disagree” - perhaps this thought often comes to your mind. You have, of course, the right to have an opinion different to the reviewer, however, that simple of a statement cannot be used as an argument. What you can do is set your concerns straight in a response, but before doing so, stop and think, “Is the other side right?”
It’s great if the comment includes a link to an article or documentation on why something is an improvement, but not every reviewer adds such details. If they don’t, you can always ask why they think their solution is better. If they do, don’t immediately treat it as proof that you cannot go ahead with your own solution. The reviewer only wants to show you their reasoning for why something might be done in a better way. Remember that the same applies to you, you can also include such a link in your response.
This is the perfect moment to pause and I’ll say it again - the reviewer is not your opponent. In fact, it could turn out that both of you are right. There is no simple solution to the problem of differing opinions, but be sure to talk with each other. If you can show the other person your way of thinking, sometimes that’s enough to resolve the discussion. And vice versa, maybe your reviewer can show you their line of thought and provide arguments that will help you decide on the solution. Sometimes, we need to go beyond the reviewer-writer relationship and agree on a joint solution, while understanding the problem to move forward.
If you find yourself in a stalemate, get an outside opinion. There is bound to be someone in the company who can help you with resolving the discussion. The third person might notice something both you and the reviewer missed and suggest which solution is better. They can also add new arguments to make your code prettier and more efficient.
“I don’t have time for this! We need to deliver our sprint before tomorrow!”
This might be another thought that pops up. Again, this needs to be discussed. After examining the problem closer, it could turn out that the change is not as quick to implement as it first appeared and engages more time and resources. It’s perfectly fine if you agree that the issue can be resolved in a separate task. Just create such a task and communicate to all interested parties when you’re going to implement it. Remember to add a comment in your code saying: "TODO in task XX-1234".
Although, before you do that, make sure that both you and the reviewer have thought the problem through and are in agreement that not having a fix in place immediately will not negatively impact the operation of the program.
Ask yourselves: How big is the issue? Could it block the development process in your current sprint and hinder your other tasks? It could, and you still don’t have the time to fix it?
What you are saying is that you want to send this feature or fix to your customers even if there’s a possibility that it might not work properly. The question then is, how quickly will it come back to you or one of your teammates and you’ll have to sit down working away at the problem again. With an added pressure that a hotfix is needed urgently, would you really want to go down that route? I don't think so.
The same applies if what you’ve done in your MR is “just” code refactoring. If you’ve made changes in a file and can see something suspicious in the area you’ve worked on - diagnose it, don’t leave it for later. You can ask the code owner why they wrote it the way they did, and what their thoughts might be about the solution you’d like to introduce. Suggest changes, remove unused code, simplify readability, and do everything that you can to improve the code.
The Boy Scouts have a rule:
“Always leave the campground cleaner than you found it".
It's similar for us:
“Always leave the code better than you found it”.
Apply this rule whenever you can!
The last, and probably the most important, thing is - be nice!
This goes for both sides, whether you are being reviewed or are the reviewer. In regards to this, I would like to say that "being nice" does not mean the same to everyone. Sometimes, I come across simple statements in the comments, for example: “fix typo”, “remove unused const”, or “make this condition more readable”. I’ve never found these offensive, but recently, I noticed that there are people who do. So, when you see such curt comments in your MRs and you don’t like them, be sure to speak to your reviewer about it. Until you do, he may be unaware that it’s something that bothers you. I recently had this type of situation and thought it over. I then changed my comments from “make this condition more readable” to “What do you think about making this condition more readable?”. If someone prefers it this way, then it’s not a problem for me. So if your end goal is to improve the communication with your reviewer, think how much you really find it to be a problem for you.
I hope you enjoyed this article and you would be able to use some of the tips above in your work.
Remember that everything can be solved with good communication 😉