Code review, why it matters

This is the first post in my series of blog posts about some of the bad codes or bad practices I’ve come across. Note that this is my personal opinion and some of the things I mention could be debatable.

Post 1: Code review, why it matters

Post 2: Not including JS, CSS and static content files in project

Post 3: Let’s just not unit test our code

Post 4: Don’t pass dependencies into helper methods

Post 5: Inline SQL query statement in static helper class

Today I want to talk about code review. To me when I review someone’s code, first thing I look for is if their changes or implementation is right and whether there is a better solution for it. I’d also look for any potential bugs or issues and any coding standard violations. Code review will help preventing bad code and defects going to master branch, hence it is very important to review others’ code properly and carefully. In my team, we have to have at least half the team review and approve the pull request before it gets merged to master branch. I’ve had experience with some team members who would wait for the lead architect to review the code changes first and then they just come in and approve the changes immediately without reviewing it, thinking that it should be good if the lead architect approves it. This is no different to having just the lead architect reviewing the code himself/herself. The lead architect is also human and he/she makes mistakes or just might miss something. It’s a bad practice reviewing code with that attitude.

I’ve also seen some people just briefly reviewing the code and approving the pull request, then come back and reviewing the changes after it has been merged. This has resulted in many defects going into the master branch, some of them are quite obvious if they put a bit more care when reviewing the code. Their reasoning for doing that is they looked through the code and saw no problem but did not actually understand the logic in the code, so they approved it so it can go into master branch and testing can start, then they would go back to the changes and tried to understand the logic. When I asked why approving the pull request if you don’t understand the changes, their excuse was it would take too long if they tried to understand the logic. To me that is an invalid excuse and not acceptable. If the logic is difficult to understand, you should spend more time reviewing it. It would definitely take longer if there’s problem in the logic because you would have to create another task, have another pull request and go through code review. Also defects will cost a lot less to the business if they are caught early.

Another thing I’d like to talk about is change requests in code review. When someone requests changes in my pull request, I see that as an opportunity to learn. As mentioned above, pull request is my implementation and solution to the problem. If the reviewers request changes because they think that there is a better solution, then we’d discuss it. If the reviewers are right, then I learn something and if I’m right then they’d learn something. And if they pick up a defect, I’d appreciate it even more. I would question it if all my pull requests get approved straight away.  I don’t see change requests to mean that the pull request fails. However, I’ve come across people who take change requests to mean it’s a failed pull request and would be offended when someone requests changes on their pull request. There were times when they were not right but refused to make the change and told me to pull the branch and fix it myself. That is not the right attitude and will negatively affect the team and the code quality.

In conclusion, code review is important because it helps preventing bad code and defects going to master branch. It should be done properly and carefully and have a right attitude about change requests in pull requests.