The missing chapter of Code Reviews

Originally Published at - https://www.maddhruv.dev/blog/The-missing-chapter-of-Code-Reviews

Code Reviews have been an integral part of Software Development Lifecycle, but lately this topic has been missing from our engineering books. This document covers some aspects of why we review code, how we should review code etc.

Why we review code?

The basic intent behind code reviews is that any code changes should be first reviewed by individuals before getting merged/pushed to the main branch of the codebase. The actual purpose of code reviews can be one or more of these, but not just restricted to these, it totally depends upon your organisation, the software requirements, legal and security compliances etc.

1. Improve the Software Development Life Cycle (SDLC)
   1. Catching possible bugs and errors beforehand - although it is proven that code reviews don’t guarantee bug safe code, but using specific tools and processes one can eliminate obvious bugs - for example undefined/null checks.
   2. Suggesting improvements - having another eye reading your code can give different aspects on it’s maintainability, scalability, reusability, - to ensure that the code you are writing can also be leveraged by others.
   3. Enforcing code styles - With larger teams and bigger codebases, code styles is a very common thing, so that the way code is being written and formatted is similar for everyone and is easier to read and write - like indentation, having semi-colons or not, tools such as linters and beautifiers can help you enforce that easily (more on that later)
2. Compliance - Some organisations and projects have certain compliances to adhere to before shipping any code into their main pipeline - for security vulnerabilities, test coverage, - one such compliance being the PCI Compliance. Almost all major projects and organisation require at least one review before merging any code changes - so as to ensure that no unknown or un-tested code is being shipped.
3. Encouragement - When someone write a new piece of code, it is also a reflection of their thought process and idea - how they see this function or component should look and behave like, also encouraging others to follow similar patterns or maybe learn and adapt something better from the code.

Recommendations on Writing Code

These are some of the recommendations around how you should/can write code to improve your chances of getting merged without much hindrance 🥅 and also by without making your reviewer scratch their head 🤯.

My personal experience: I followed most of them 😉 in my projects and invited the least number of review comments (obviously not because people are afraid of me 👻) but because, it just works - hence I end up completing the last half of my Jira tickets faster - merging the PRs.

1. Keep changes as small as possible - By small I mean as contextual as possible, don’t confuse the other person on the court with random changes which are in no way related to your original intent for the code changes. Your Pull Request or Review Request (whatever you call it) should be small and logically grouped - and should prove only a single thesis.
   1. Okay! First of all understand this - ‘it’s fine to have 5 different PRs for shipping a feature’ - nobody is ever going to blame you for raising so many PRs and increasing their CI costs - if they do, share this blog with them. Just making sure each of your PRs or individuals changes are clear and testable, it shouldn’t be like that you are raising multiple PRs for the sake of raising multiple PRs. Let me give you an example - assuming you are building a web page for signup, you can easily split the feature into - 1. the actual form component - inputs, validations, styling etc (make sure they are testable via mocked inputs, storybook, unit testing etc.), 2. adding the signup APIs, logic for redirection, creating various functions to handle the stuff - again, testable, and lastly 3. linking both of them together - the UI and logical components. This way of splitting your PR also ensure least amount of risk in breaking anything and the reviewer has better insight into what is happening.
   2. Continuing to the same point - do not raise anything everything in one PR, it is a recipe for disaster - literally disaster 💣, if any small piece breaks in it, it breaks the whole PR - breaks the whole set of changes.
      1. Imagine having a PR with 27 files changes, being shown on multiple pages or scrolls, the reviewer has to keep so much on information in his small buffer memory, around how these multiple functions were written are they used properly everywhere or not. The context jumping should be as minimal as possible - just don’t add random script changes in a feature PR with other random bug fixes.
      2. Reverting any breaking or unwanted changes will be difficult, assume your PR contains two or more set of changes 1. Going to Mars and 2. Going to Moon, what if the mars mission fails and you want to revert the changes, in such a PR/commit you would have to revert both the changes, even though moon mission is working fine.
      3. The general idea is to set some ground rules within your team and org, is to never have PRs with more than 10 files or 1000 line changes (exceptions to be noted) - in my previous company we had built a github action that just sends some gif if your PR is long enough and declines it automatically.
   3. Solve only 1 Ticket/Bug at a time, that’s it
2. Leave descriptive comments - How does your code reviewer know what are the changes? How do they know about the changes you are making are necessary for the feature or bug fix? An explanatory description with your PR and changes is very crucial part of the code review process. It just not only help the code reviewer with the context but also helps other onto what code changes are being shipped.
   1. Add a Changelog description accommodating all small and big changes, wherever required add steps to test your changes, if there are next steps to the changes mention those as well, the goal is to educate the other person with all the information around the changes.
   2. Add relevant screenshots and demo wherever required, certainly for UI changes, just by looking at the code or css changes, one cannot visualise the changes. Add as many screenshots as possible - dark mode, light mode, device layouts, all, don’t feel it as a burden, feel it as a responsibility.
   3. Mention about any alternatives you tried and failed/didn’t chose, communicate it, if you have tried some other way to make these changes, maybe using some other algorithm, resource or library and why it didn’t work, so that for the next time, when someone tries it, they won’t spend their time trying out things which don’t work for the case - again, educate the team.
   4. Lastly, add inline comments for changes which are not very obvious to understand - like any refactoring, type fixes etc.
3. Good practice but not required - Semantic Commits - look at these example for commit messages - fix, ., remove code, don't know how this works, it just works

Commit messages have a purpose, so that when you look back in your history, you would know what are the constituents of a particular commit and you won’t have to diff each and every commit to look for the changes you require. I generally follow the Conventional Commits and Semantic Commit Messages to commit my changes, and there are tools which can help you write these beautiful commits like commitizen, still it is your responsibility to add a meaningful commit message, no matter if you follow and semantics.

1. Don’t leave comments unattended - whether or not you are accommodating the requested changes, do mention it, reply to the comments, don’t just ignore them, if you are not making the changes, clearly mention why - if there are follow up tickets, add links to them, if there were offline discussions, mention them. Leaving comments unattended not only dictates that you are careless but also adds a red flag onto your next PRs, and you don’t wanna do that.

Recommendations on Reviewing Code

I know I know, too much content on the first section as well, let’s keep this short and concise. We have talked enough about how to raise review requests, let’s talk about the other side as well - how to review requests. We all hate when those senior engineers block our pull requests for any random reasons, without justifications, without elaborative comments, just block. This is for them as well, so share with your senior engineer 😜.

When reviewing any code or pull requests, just keep these in mind and review with utmost neutrality.-

1. Be explicit and clear about what questions you have or what suggestions you are making, communicating over a comment based user experience has never been that efficient, you might try to say something else and the other person might understand something else. Always over communicate, the worst thing you can do it to confuse the receiver onto what changes you want. Basic tips 💡 -
   1. Add reasoning to what and why those changes are required or recommended.
   2. Add code snippet suggestions, if you want things to be of certain format or using some specific ways
   3. Add links to the documentations around various APIs and methods, the other person might not be aware of that method.
   4. Make it clear if the comment is just a suggestion and not a blocker for the review request, the other person might spend a lot of time understanding and implementing the changes and might not even add any values to the code.
2. Set some ground rules across your team/org.
   1. When to approve and request changes on a PR. Recently I faced a very strange issue with a Git review tool my team uses is that anybody (even the requester) can mark the comments as fixed and without even confirming if the changes look good or not, they can seek approval from others (maybe their close friends) and merge it, even though the original reviewer hasn’t approved it again after the changes - very very harmful ⛔️.
   2. Don’t make it seem like that you are blocking them - some changes are critical but some are not - they are just like a good to have or nitpick, add comments but don’t make the PR blocked by just them, maybe you can ask the requester to accommodate those changes in a following PR.
   3. Only reject PRs when there are obvious bugs and non standard code - maybe adding some vulnerabilities or conflicting with your code guidelines.
3. Know when to take a discussion offline - add comments which can be historically crucial and useful when looking back, but never overstretch a discussion on any PR. If any discussion is overstretching that simply means there are conflicts with ideas and the way your team writes code - take it offline - have a meeting setup with the team and draft down opinions and come to a common ground onto how that piece of code should be, overstretching any discussion on a PR can be frustrating for both the parties and eventually impacts the deliverables.
4. Don’t just keep on creating backlogs - remember those I will address this in next PR, let me attach a follow up Jira ticket , well for certain cases, it is fine, but not for all, when you create a follow up ticket, you create a backlog, and tracking those tickets, prioritising them, is a piece of effort in itself, I have seen people mentioning some important changes to be addressed in next pull requests but are never, ‘cause once a breeze is gone, it’s gone. Strictly do not create backlogs and if it is necessary (maybe of some urgency to ship the main PR), prioritise the follow up and work on it.
5. Never mention to the reviewer that this PR is hanging for a while, let it pass this time. I hate it when someone says this to me, if your PR is hanging for a while, was it because of the reviewer? probably not, many reviewers I know only allot a specific amount and slot of time for PR reviews and in remaining time they like to do their own work. It is certainly the requester’s duty to address the comments and follow up on them. I have seen people not addressing comments for 2 days and then mentioning to the reviewer that it is urgent to merge the pull request, well I personally don’t entertain such people and pull requests.
6. Niche things you can and should look for:
   1. Cleaner Code - naming conventions, comments, unwanted code, etc.
   2. Forms of Input - how does the API accept requests, how the data is structured etc.
   3. Test Coverage and Cases - pick up cases which certainly require test cases or are missing coverage.
7. Pull in people who have more context on the piece of code - sometimes you as a requester or primary reviewer might not have the full understanding of the code that is being changes and hence increases the changes of breaking something here and there. Pull in the team or people who have previously worked on that code or knows about it better than you - I like the way how Code Owners on Github works.
8. Lastly, don’t be political - hey he added so many comments on my PR last time, I will also add some random, irrelevant comments to him and block his PR, I can also show him my powers. If you have even 1% of this psychology, quit your job, go to a country side and start something different, something that doesn’t involve working with people and teams.

Tools you can use

Well there are a lot of things you can automate in code reviews, certainly tests, code styles, etc etc. There are certain tools that can help you enforce that and make the review processes faster and easier:

1. Lint - Tools such as ESLint and Stylelint can help you enforce various rules in javascript and css - code styles, restricting use of certain methods etc.
2. Code Styles - Tools such as Prettier can help you apply certain code formatting with ease.
3. Automate your tests
4. Lighthouse - Audit your web performance etc.

Well enough for today, I am tired and going to sleep now, do share with your team and educate people.

Originally Published at - https://www.maddhruv.dev/blog/The-missing-chapter-of-Code-Reviews