Code review first look and best practices
Code review nowadays is a worldwide standard in software development or software engineering. It consists of the task of doing a code review to a fellow developer so that once it has been validated by one or more people (depending on the size of the team, the methodology proposed by the team or related factors) that code / merge request / pull request advance in the development process to be able to reach the next steps, like QA, Staging, or even Production. It is a practice that can be carried out regardless of the type of software being developed: an operating system, a commonly used library for Java, a website, a game, etc.
When conducting these reviews, several things are expected to happen:
- Finding bugs at an early stage of development: An external view always gives good perspective to find bugs that the other developer did not find at the time of writing (we tend to get too involved sometimes in what we are doing and sometimes we can forget basic functional details and it’s great to have someone else helping us to avoid it)
- Sharing knowledge: From my point of view and many colleagues too, it is great to be able to see how another person faces a problem in order to ask ourselves if we would have done the same thing, maybe worse or even better and also you have the chance to ask about certain decisions made directly to the person who wrote the code.
- Distribute knowledge related to the business among team members: Something fundamental in a development team is to be relatively calm that a feature developed by someone is also known to the rest of the team. In this way, if for any reason the person who wrote that code is not available (due to illness, vacation, or whatever reason), any of the team could resume that feature without major complications. Or at least this should be possible as long as the person who did the review has done it thoroughly and made the necessary questions to understand the background of the problem and the implementation decisions.
Unfortunately, many developers think this practice is unnecessary as some see it as a way to "watch over" what they do on a day-to-day basis instead of seeing it as a useful tool to make progress on our career as a software developer or engineer.
Learning should be the first goal for developers in the implementation of this practice and the first goal for companies is to help finding / avoiding bugs as quickly as possible to deliver quality software to its clients.
The ideal thing to do when starting these practices is to make it clear to all participants both the objective of the practice and the way they should be doing it.
For example:
- If a colleague leaves a comment about something they think could be improved and does not receive a response, it can lead to this person thinking that their contributions are either not very well seen by their peers or that the people they work with do not believe that the practice is useful. For this reason, it is important at least to answer the messages received on the platform.
- If there is a person who considers that the change is not ready to be merged and other people consider the opposite, it would be good to spend a few minutes (or the time necessary depending on the particular situation) to be able to reach a consensus and that everyone is satisfied with the decision.
- To save valuable time from our colleagues, the ideal would be to be able to leave all the documentation that we think relevant so that they can understand the objective of what we wanted to achieve in the merge request, either by leaving the corresponding ticket, a screenshot or various comments detailing the decisions considered for the feature implementation.
- Take into account that if previously there was no agreement on the coding standard or way of structuring code, do not start leaving many comments saying things like "leave a line break", "add space when closing the brace in this function", " reformat the code ”, etc.
- What can be done is to tell the team the need / idea to start analyzing those things to see what the rest think and analyze if it could finally be implemented or it seems like a waste of time for any reason.
Another interesting practice on the subject is not only letting the people who maintain the project be the only ones to comment on it.
For example, if you are developing a backend with kotlin, an android developer could easily join the code review (perhaps without the possibility of him being in charge of the approval, or yes, it depends on each team) to analyze the use of certain common language tools that are used on all platforms.
You could even comment / ask about decisions if you are more or less on the subject (whether you did previous work on that platform, took a course, or whatever. The possibilities are endless).
This practice can help empower team members who want to learn new things (be it languages, tools, libraries, or eve people who is trying to start programming for a different platform)
In some tools it is possible to determine / configure code owners. The code owners would be the people in charge of maintaining that code. This may mean that they are the ones who must make the decision to accept / reject pull requests (but not necessarily the only ones to leave comments). And if the tool does not allow them to be configured, there can always be some verbal agreement outside the platform (which is not very common in large teams to avoid problems of merge requests approved by mistake or similar situations).
If you are interested, you can find many examples of pull requests or merge requests in the different most popular repositories on the web, just like GitHub (https://github.com/ ), GitLab (https://gitlab.com/), BitBucket (https://bitbucket.org), Assembla (https://www.assembla.com/home) to name just a few.
The truth is that it is a very interesting and extensive topic to write about so I’ll surely continue writing in the blog about different ways to implement it or good practices to keep the sanity of all the development team members.