The reviewing of code is indispensable when working on any enterprise application. I have worked on codebases that have no review process where developers freely merge straight to master, and my comment on that is, “When we lose our principles, we invite chaos”.
Pull/Merge Request (PR): requesting to have your piece of development on a project merged into the main branch. The main branch is the culmination of all development and is the source of truth. A developer attempts to get their code approved and merged into the main branch so that their code may become a source of truth for others.
1. Improving
The most important aspect of reviewing is the sharing of knowledge and betterment of the team. A developer who continuously makes the same mistake will be refined through a review process (as comments are made on the repetitive mistake). This is a very small example of how the review process betters developers. A PR is also a convenient way for a developer to teach. Example: if I see code using a forEach in JavaScript that assigns a boolean value to true and breaks, I will tell the author that a some function is more suitable. Reviewing should not only be the correction of errors; developers must share their knowledge and suggest improvements. Greedily hoarding knowledge as a developer is futile.
2. Informing
A PR also informs the team of changes to the codebase. Without a review process, code is merged straight to develop (default branch). What if dev Foo is working on feature A and dev Bar merges in a pile of nonsense also for feature A? Foo will have conflicts and may well continue working, totally oblivious to the (bad) changes made and merged. Perhaps in this scenario however, the onus is on Foo to regularly check develop for changes.
Another example: a senior discovers a new way to implement an accordion on a UI project. If one fails to review the change implementing this improvement, one will continue to implement it incorrectly. The correction will only take place once a PR is opened (and the senior informs the dev of the improved accordion implementation). While this is not a calamity, time could have been saved if the dev just reviewed the senior’s PR. Interestingly, this contrived example indicates how a review process — obviously being performed by a competent team — can catch issues sooner or later.
3. Enforcing
A contributing document may be the best way to enforce standards, along with static code analysis, like a linter. But there may be things one is unable to mention in a contributing file. And there may be things a linter is unable to enforce/catch. A review could be viewed as the last line of defence. Before the review process begins however, all checks should have passed. If pipelines are configured, linting, building and testing must pass. After that one can check whether guidelines in the contributing doc have been adhered to.
Only after all these checks have passed shall a human spend time reviewing the code. Enforcement of standards should be as automated as possible. Now a developer will receive real, human feedback on their code. And many things could easily come from the human review! The point being made here is that even with all the tools, pipelines, jobs, and documentation, things can be missed and things can be done incorrectly. The last line of defence in this case is also the the most crucial. Robots are close to doing what we do but not quite yet.
4. Readability
At a glance, a reviewer should be able to understand what the author of a PR is attempting to achieve. A reviewer at a glance however, cannot be expected to understand how the author has implemented the desired behaviour. At least if the code is readable the reviewer is able to inquire into the how’s of the code. This is not to say that the reviewer does not need to understand how the code works. Neslo enforces the practice of breaking up tasks (and subsequent PRs) into small chunks. This makes the PR and the code within it much easier to consume, review, and understand. One may be inclined not to review when the diff is 48 files… Massive PRs and time constraints can be limiting. A reviewer does not always have ample time to thoroughly examine a PR. Finally, following a standard of high readability can also lead to the inadvertent creation of maintainable code and the setting of unspoken standards.
5. Understanding
Even though a reviewer may not understand the intricacies of every PR, they at least cultivate some level of understanding of other features. Example: if I am working on the teachers part of a school administration application, it is certainly beneficial for me to understand the way the subjects feature works. While I may spend no time directly developing the subjects feature, by having a part in the review process, I at least have some level of understanding of it. This describes a flow of understanding travelling from the author of a PR to the reviewer(s). Conversely, understanding and enlightenment may also flow from the author downwards. I would imagine this is the direction of flow most of the time because typically it is a senior reviewing a junior’s code.
6. Including
A developer’s title should not dictate the importance of their contributions (to a PR). Neslo has a philosophy that the reviewer does not always need to be in a position of seniority. Peer code reviews are excellent. It’s a great way for a developer to become comfortable with the process of reviewing (EVERY dev should review code). There are millions of ways to reach the same end when coding. Reviewing allows one to see all the quirky and beautiful ways people reach these ends. It may also allow one to recognise patterns and rules present within a code base that are not always explicitly stated.
Closing
As mentioned under inclusivity, there are millions of ways to achieve the same outcome when programming. While this is truly amazing it can also be daunting. One may feel they are never doing something “the right way”. And one may feel nervous to code. As one of the leads here at Neslo says, “Do not be afraid to code!”. What I would like to add to that as well is do not be afraid to read code! In my mind there are many ways for me to achieve an outcome, now imagine how many different ways there are in different minds! If you’re not constantly attempting to interpret and learn from other people’s code, you will swiftly become fixed in attitude and position, and the word for that is ossification. And stagnation is the mightiest foe of the developer.
