Code Reviews (or Peer Reviews) are a pivotal cog in the wheel for producing high-quality software products. Outlined below are our thoughts on the topic, with emphasis on alignment towards Terem’s Philosophies.

Introduction

Pre-reading

Code/Peer Reviews as concepts have been around for a long time, and a plethora of content (by folks way smarter than us) already exists in the public domain.

Salient Features

Code Reviews have ample documentation online. How are we different? Listed below are the high goals and the reasons why we documented this:

• We needed to align Code Reviews to our Company’s culture and core philosophies;
• We needed to standardise our Engineering practises into an Engineering playbook;
• Our engineering culture revolves around agility. Therefore, apart from using the Code Reviews maker (Code Creator), checker (Code Reviewer), and model, with a little love and discipline in explaining context, we see great value in involving other indirect roles like Delivery Managers, Business Analysts for using Code Reviews as a fluid leading indicator to measure the progress of a feature. Also, in the use of this bite-sized amalgamation of Business Context  + Technical Reviews to act as a vehicle to swiftly bridge knowledge gaps for New Team Member Onboarding, Code Handovers to Clients, Quality Assurance, Operations, etc.

Assumptions

Most of the content that follows is written with the Pull Request (PR) features provided by tools like Github, Bitbucket, etc. in mind.

Roles

Core Contributors

1. Review Creator

Developers, QA, Architects will be primary code contributors and act as review creators.

Things to Keep in Mind While Submitting a Code for Review:

• Provide reference to the necessary documentation eliciting the user story & the acceptance criteria
• List any assumptions, gaps and edge cases you may have encountered while working on the code
• Attach short bullet point explanations of the context
• Attach relevant screenshots of key areas/decisions for the code
• Optionally, attach a video of your work in action along with supporting commentary
• Optionally, attach a video of the code, its design and other architectural decisions taken
• If the feature being developed is still incomplete, clearly state that (by creating a Draft Pull Request (PR)) and the intention behind what exactly needs to be reviewed. We encourage reviewing code often, in small increments, to avoid cognitive overload to the reviewers. Remember that reviewers are working on their own features and have to switch context in order to review your code. The extra effort to publish small incremental changes might help improve speed and delivery efficiency
• As much as possible, ensure that the code is in a workable state, runnable on the reviewer’s local machine, and all surrounding automations (linters, style bots, tests – unit, functional, regression, etc.) are passing before expecting a review for your submission
• Introspect the submission as a Reviewer; in other words, put yourself in the reviewer’s shoes; ensuring you have ticked all the boxes
• Strive for at least 2 reviews for your submission, with a mix of Junior and Senior members, covering both precision and knowledge transfer

Things to Keep in Mind on Receiving a Review From Someone:

• All feedback is good feedback. Criticism is also feedback and is good
• There are no stupid questions
• Problem-solving is about perspectives, and understanding someone else’s perspective might help you improve and improvise your own
• Be ready to help the reviewer if they get stuck trying to run your code. Jump on a call, screen-share and help them get the code running. There might be powerful hidden insights to be gained. It may be in some automation, documentation, etc.
• Don’t get emotionally attached to your code. If a reviewer’s comments feel upsetting, take a break and swallow the Red Pill. Be equanimous. Have strong opinions, but hold them weakly
• Appreciate good work. Reviewing code is sometimes more complex than actually writing the code. Leave Kudos for something that impresses you. People are most important

2. Reviewer

Developers, QA, Architects will act as Reviewers.

Things to Keep in Mind While Performing a Code Review:

• Ensure that you spend time understanding context before jumping into the code. Provide feedback to the contributor benefits other reviewers. Don’t hesitate to spend a few minutes with the project’s Business Analyst(BA) to gain insights into the business context
• All feedback is good feedback. Criticism is also feedback and is good. Strive for quality outcomes
• There are no stupid questions
• {sarcasm} ∩ {code review} = ∅ – In non-mathematical terms “No sarcasm please”
• Be respectful
• Provide references/examples of your suggestions where possible – the closer to context, the better.
• Appreciate good work. Leave Kudos for something that impresses you. People are most important
• Have strong opinions, but hold them weakly
• Leave your feedback in 2 working calendar days. For example, if the request came in on a Wednesday afternoon, make sure to leave your comments by Thursday COB. We encourage blocking time in your calendar daily for addressing pending reviews

What Not to Look For in Code Reviews?

• Formatting & Style (Brackets, tabs, spaces, newlines, trailing commas)
• Naming Conventions & Other standards (snake_case, camelCase, kebab-case, PascalCase)
• Test Coverage (should be automated)

In the past, industry-wide, a lot of emphasis on coding reviews was put on the above areas. Our industry has matured adequately for these concerns to be fully automated using formatters, linters and test tools available for most languages and frameworks used in the modern-day toolset. Due to failing builds in the CI/CD pipeline, such issues should be remediated by the developer producing the code.

What Should You Look For in a Code Review?

Context & Code Submission

• Does the submission provide reference to the necessary documentation eliciting the user story & the acceptance criteria?
• Is there summarisation text and/or a loom video/screenshots providing the submitters perspective of what is to be achieved?
• Is the code submission intended for files relating to the said features? Does it have work from any other features?
• Is it too large to handle for a single review? Too many files and code changes? In this case, provide feedback to the contributor that our expectation is to have frequent short review stints for more efficient review and development processes

Design

• How does the new code in the review submission fit in the overall architecture?
• Does it follow the design paradigms & patterns the team prefers? e.g. SOLID, max 10 line functions, FP style, SRP etc.
• Is the code in the right place? e.g. Code related to Orders should lie in the Orders Service. This is especially true for new contributors who are still learning about the organisation of code and will be eager to contribute while still in the learning phase. As an experienced developer of the project reviewing the code, it’s imperative you help the contributor find the right places to include their code. This not only maintains overall hygiene but helps the nascent contributor better understand the overall code architecture & organisation
• Could the new code have reused something in the existing code? Does the new code provide something we can reuse in the existing code? Does the new code introduce duplication? If so, should it be refactored to a more reusable pattern, or is this acceptable at this stage?
• Is the code over-engineered? Does it build for reusability that isn’t required now? How does the team balance considerations of reusability with YAGNI?  Or avoid hasty abstractions (AHA)?

Readability & Maintainability a.k.a Complexity

• Do the names (of fields, variables, parameters, functions, methods, classes etc.) actually reflect the things they represent?
• Can I understand what the code does by reading it?
• Can I understand what the tests do? This is vital. Tests should be used as a touchstone for understanding the usability of the code. If the tests are not readable generally is a good indicator of code smells
• Do the tests cover a good subset of cases? Do they cover happy paths and exceptional cases? Are there important cases that haven’t been considered?
• Are exceptions handled for edge cases? Has there been sufficient effort put in error handling? Remember that the code is currently fresh, and there is a high likelihood that if well-instrumented at this stage can greatly improve the operational efficiency of the overall product.
• Are complex pieces of logic well documented, commented or covered with tests?

Functionality & Performance

• Does the code actually do what it was supposed to do? If there are automated tests to ensure the correctness of the code, do the tests really test that the code meets the agreed requirements?
• Does the code look like it contains subtle bugs, like using the wrong variable for a check or accidentally using an ‘and’ instead of an ‘or’?
• Does the code use the appropriate data structures to solve the problem? 
• Does this piece of functionality have any hard performance requirements? Or does this code negatively impact any existing features?
• Are Database connections, Network calls, Parallelism, Caching considerations adequate?

Tests

• Does it adhere to the codebase and/or teams test approach?
• Are there tests for this new/amended code?
• Do the tests at least cover confusing or complicated sections of code?
• Can I understand the tests?
• Do the tests match the requirements?
• Can I think of cases that are not covered by the existing tests?
• Are there tests to document the limitations of the code?
• Are the tests in the code review the right type/level? – Unit vs Functional vs Integration tests
• Are there tests for security aspects?
• Are there any tests that I can contribute to better the submission?

Miscellaneous

• Is there potential security problems with the code?  – authentication, authorisation, secrets etc.
• Are there regulatory requirements that need to be met? – auditing, logging etc
• For areas that are not covered with automated performance tests, does the new code introduce avoidable performance issues, like unnecessary calls to a database or remote service?
• Does the author need to create public documentation or change existing help files?
• Have user-facing messages been checked for correctness?
• Are there obvious errors that will stop this from working in production? Is the code going to accidentally point at the test database, or is there a hardcoded stub that should be swapped out for a real service?

Consumers

Engaging more than the engineering team in the code reviews can increase collaboration in the team and help the delivery managers manage stakeholders and dependencies.

1. Delivery Managers

Delivery Managers are the glue holding the overall engineering lifecycle of software delivery together. Their inclusion in code reviews can allow greater team collaboration and de-risk stakeholder communications or management of dependencies.

What Does a Delivery Manager Get From a Code Review:

• Validate if the business context has been captured accurately from a technical perspective
• Code Review context can act as an excellent source for Delivery/Project Managers to understand the technical complexity of a feature being implemented; this provides a leading indicator of delays
• It can help in decision making of taking a particular item out of scope as it might be too complex to cover but may not add direct value at this juncture (enable mid-sprint prioritisation)
• Understand gaps in the estimation/planning poker session vs actual implementation. For example, if testing complexity was not previously considered, integration will be painful and needs to be considered beforehand during the estimation sessions
• Gather high-level technical context to enable the team to alleviate impediments like dependencies on other teams
• Using the screenshots and videos as a reference provides context to plan for the end of sprint stakeholder demos 

What Contribution is Expected From Delivery Managers:

• Provide feedback to ensure that the review context is well-groomed
• Appreciate good work; leave Kudos for something that impresses you. People are most important

2. Business Analyst

Business Analysts are the custodians of business knowledge and act as the bridge between business and technology.

What Does a Business Analyst Get From a Code Review:

• Validate if the business context has been captured accurately from a technical perspective
• Use it as a leading indicator of how the feature will be delivered and clarify gaps

What Contribution is Expected From Business Analysts:

• Provide feedback to ensure that the review context is well-groomed
• Appreciate good work; leave Kudos for something that impresses you. People are most important

3. Other Interested Parties

This is a role involving members of the team whose consumption of the Code Reviews would be geared towards knowledge osmosis (think New Team Members, Code Handovers to Clients, Quality Assurance, Operations, DevOps etc.). Although the involvement of this role is primarily read-only, we give a lot of emphasis to the added value that it provides. Experience has shown that onboarding and knowledge transfer tasks can be challenging to manage in our craft, where the deliverable is in a constant state of flux.

What Do Interested Parties Get From a Code Review?

• Time travel various different Reviews relating to Feature/User Story/Epic to understand its evolutionary growth
• Use Reviews as an ancillary source of documentation
• Gain insights into being a Core Contributor

Further Reading

Shameless inspiration from the wise words below:

• Why Code Reviews Matter?
• Effective Code Reviews
• Google Engineering Practises
• What To Look For in a Code Review
• Mind-map Explaining Code Review as a Concept

---