Productive Peer Review

A while back, I wrote a piece on the importance of code review and the role it plays in good developer practice, as well as its impact on team dynamics. This article aims to extend that work and tackle the next logical step - the internals of a good code review.

Code review should be a principal means of exchange for information, ideas, and best practice. It is during code review that there is always a concrete problem to solve, and a set of defined parameters for the solution, all of which give reference points for informed and useful discussion.

Keeping Perspective

At all times during a code review, there are four questions that must be kept at the fore:

1. Why am I reviewing this code?
2. What is the purpose of this code in the wider context?
3. Can this task be accomplished in a better way?
4. Who gains from this code review and how?

The first question might seem a little strange and odd, but it forms the prism through which the entire process will be reviewed; it is the foundation of one's perspective.
This is because an inexperienced or new-to-project programmer reviewing the work of a seasoned veteran will need a different mindset to conduct an optimal review compared to vice-versa.

Question two is usually second nature to more experienced developers, but one that junior developers will frequently forget (I personally have lost sight of this context whilst reviewing in my early career on more occasions than I'd care to admit). Keeping this context in mind grants three benefits.

• It cements the reviewer's knowledge of a continually changing code base.
• It provides a layer of reassurance that the code solves the problem that it was intended to solve.
• It provides an opportunity to grasp the wider implications of the work being carried out - for example whether this change is going to produce an incompatibility between this service and another.

The third question speaks to something rather fundamental - that the process of improvement should always be at the forefront of review. It is likely that the author read and re-read their code to make sure that all was well, but invariably things get missed, or the reviewer might have an extra piece of knowledge that allows an objective to be achieved in a better way. In general a reviewer should be looking to see if the solution strikes the right balances between efficiency, readability, resilience, and usability.

The last question is a sanity-check for any/all comments placed upon a review.
By asking who this helps and how, the accidental posting of comments that are unhelpful can be reduced; as well as bringing clarity to the intentions of a comment - namely its target audience and what information is sought/provided.
If an answer to this question's parts involves "nobody" or "badly" respectively, then perhaps something else should be written instead.

Mindset

In order to adopt the correct mindset for a review, one must first answer question number one - "Why am I reviewing this code?"
The answer to this goes beyond the surface response of "because I have to" and must instead speak to the objectives of the review.
These are likely to be a combination of learning, mentoring, safety, and optimisation, depending on the reviewer's skill level and experience on the project.

Reviewing the code output of a more experienced developer has tremendous returns. When almost any other subject is taught, an emphasis is placed upon reading the great works of that subject. Programming is no exception. In order to write good code, one must first read good code — A lot of good code. Undertaking code review provides this opportunity in abundance, with the added advantage of being encouraged to ask the author as many questions as necessary.

Conversely, a more experienced developer may seek to use code review as a golden opportunity to impart pertinent knowledge to less experienced developers. Learning principles in the context of one's own code and work, makes it much easier for the knowledge to be retained than in a contrived setting.

Consider One's Audience

If a reviewer holds back or restrains themselves when providing advice and feedback, then valuable information is lost not only to the author but also to the wider audience.
It is important for the reviewer to remember that the conversation is not solely between them and the author; Asking questions helps all collaborators understand the purpose and reasoning behind a change.

If the reviewer is part of a larger team, then it is very possible that questions that are in their mind would be of great interest to the other members of the team as well, who might well be watching the thread or reviewing the pull request themselves.

As a reviewer your insights and wisdom are valuable to the whole team.

Magnanimity & Humility

Granting the author the benefit of the doubt leads to a more productive review experience for all involved.
When an author feels that they or their work have been interpreted in the worst possible manner, it is likely that they will become defensive, which is not conducive to informative and honest conversation about the code.

When reviewing code it is always best to make certain assumptions that should remain in place until disproven:

• the author has done their best to complete the task at hand;
• someone adding a question is either trying to improve or trying to understand;
• someone leaving a comment that is critical is only trying to help you improve and grow.

However, whilst these assumptions are easy to state, taking them to heart can be difficult. It is always advisable for the author and reviewer alike to make these assumptions as easy as possible to maintain. Authors should run basic sanity checks against their code (including the "what to look for" list in this article), whilst reviewers should phrase their comments in the most open possible manner; for example asking why a particular approach was taken is a better way of getting the author to think about its merits than simply demanding a rethink. If an author is required to justify their decisions to another person then that alone can often be enough to produce a superior approach.

Further, it is absolutely key to remember that a comment left on a code review can be wrong, even if it is from a more senior developer. If something is said that doesn't look right, the author and/or other team members have a duty to speak up.

Some Best Practices

What to Look For

As a non-exhaustive list of things to look out for in a code review, one should check that the code is:

Readable

Is the code easy to read? Ultimately it could well be the reviewer themselves who next has to edit this code, and that could be in several months' time. If it doesn't make much sense now, it's going to make even less sense then.

• The code should be structured well, and not repeat itself.
• Public methods should be documented.

Maintainable

How easy is it going to be to maintain this code in the future?
Much of this resides in two key factors - how well the code is designed (which also speaks to readability) and how extensible the code is. If the structures of the code cannot easily accommodate a new addition (say a new option) then a comment requesting a refactor is in order.

• Magic numbers should be avoided.
• If the project demands it, all strings should be localised.

Tested

A good rule of thumb is to check whether

1. Every new change in the code has a corresponding change in the tests.
2. Each test accurately checks exactly what it says it does.
3. The tests cover all code pathways. A coverage tool is desirable for checking this but such tools are not foolproof. They highlight whether a path has been used in the execution of tests, not whether important information about that path has been asserted against in the tests; that will have to be checked the old-fashioned way - by reading.

Accessible

Is anything in this code going to prove troublesome for users of assistive technologies like screen readers or magnifiers?

Accessibility is worthy of whole books, however some of the most common issues that arise are:

• visual-only items (like icons) not being hidden from screen readers, creating audio-clutter;
• heading levels being skipped (e.g. a document with h1, h3, h4...);
• heading1 being used more than once;
• changes in screen content are not announced;
• associated items are not semantically linked (e.g. form controls improperly labelled or a table built with <div>s (😱).

Complete

The presence of TODO or FIXME in a pull request should be a red flag. Whilst I would not say that they are sufficient reasons to reject a request outright, they do have their place and must be used correctly.
One must recognise that a TODO is a confession that the author is adding tech-debt to the project, and as such there must be a correspondingly good reason for doing so (such as work well out of the scope of the task at hand).
As good practice, any TODO that is being committed should contain a link to a ticket that describes the work that is to be done and outlines its scope.

Make Suggestions

When a reviewer wants to suggest a small scale change such as changing a string literal, or refactoring a simple method, then it often pays to use the make a suggestion feature in Github (GitLab and Bitbucket offer the same feature). This can be done by pressing the

button or by creating a code block (using three backticks) with the language set to "suggestion". For example:

The advantage of doing this is that the author can immediately decide whether the new code is a good fit, and if it is, that code can be committed straight from the conversation thread, with no need to return to the development editor.

⚠️ Warning
If the suggestion that is made requires any new imports, these will have to be added as separate suggestions. Either that or the whole change will have to be left to the author in their editor. This is because github will not auto-import something new, and the code will simply fail. The same applies if new methods have to be created - either those new methods are included in the suggestion or they must be suggested elsewhere.

Check that it Works

This may sound obvious, however the best code review in the world is entirely in vain if the reviewed code does not achieve the task it was written to accomplish. When reviewing code, pull down that branch and test it against the specified problem.
This also provides the opportunity to assess how the code responds to certain inputs and edge cases, which is especially useful if the code makes it unclear how the program will behave.
Finally, running the code affords the opportunity to test the accessibility of the overall end result.

What to Avoid

There are certain things to avoid whilst conducting code review, some of which pertain to the content of the review whilst others are more related to attitude.

The ultimate mechanism by which code review achieves its goals is to create the maximum exposure between code and experience. Without this exposure, novices cannot gain the wealth of others' experience, whilst those with years under their belt may miss out on fresh approaches.
Either of those perspectives represents a detrimental outcome for the code itself, which can cause problems that future developers spend years cleaning up.
If a developer dreads review-time, they will end up taking measures to avoid it as much as possible and the problems highlighted above will begin to creep into the project and team.

Content

Ambiguity

It is important that a reviewer's comments be as unambiguous as possible, lest the author waste precious development time after misunderstanding what was being asked.

One of the most common sources of ambiguity is when a reviewer uses too many pronouns in reference to the code under scrutiny; when there are (say) seven variables floating around within two functions, "this", "it", and "they" become far less meaningful and an author can easily get the wrong end of the stick.

Unfortunately language is a tug-of-war between the CONcise and the PREcise. In the context of code review one should unapologetically aim for the latter, and if it comes at the expense of the former, then that should be considered a price worth paying.

Incompleteness

When suggesting that an author should look up a particular approach or another part of the code that holds a clue to a solution, the reviewer needs to be explicit. At a bare minimum there should be links included in the comment to relevant documentation / repository pages.

Note:
This is not a requirement that reviewers should solve author's problems for them; neither should it be used by authors to demand such. Reviewers can be of great help, but ultimately it is the author that has taken on the task.

Attitude

Superiority

Whilst it is entirely possible that the reviewer is of a generally greater level of skill and experience compared to the author, it is equally possible that the code's author has particular knowledge not possessed by the reviewer, or has done detailed research related to the work at hand (for example they might have looked up little-known features of an external library).

If a reviewer is haughty, it can be most disheartening for the author. It is one thing to suspect or know that others are more experienced/skillful, but it is quite another to have it pointed out.

However it is also worth pointing out that there is still some responsibility incumbent upon the author - if there is a niche piece of knowledge upon which the changes rest, then the author should indicate where the resources to understand this knowledge can be found.

Sarcasm

Perhaps one of the biggest oversights in the design of HTML is the lack of markup for text that intends sarcasm/insincerity - much to the detriment of the social-media experience.
It also impacts code reviews.

When reviewing, it is absolutely critical that sarcasm be avoided, lest it be interpreted as sincere. The prospect of sarcastically 'praising' a piece of code runs the risk of the author taking the praise seriously and proceeding regardless.

For an outlandish demonstration example, see below.

If the author interprets this literally, the entire redux cycle will be broken as the actions will no longer dispatch correctly. Instead, the below would be a better approach:

Finally sarcasm is often unpleasant, whilst the temptation to wield this unpleasantness is at its greatest when finding fault. If it is used in the scrutiny of code, then the boundary between amusing and unkind can be crossed more easily than one intends.

Author Actions

The reviewer is not the only party that can make peer review a more productive process - there are some actions that the author can undertake that are just as important.

Clean Code

I will not make an exhaustive list of what is considered clean code, as there are entire books dedicated to that topic (particularly Clean Code by Robert C Martin and Clean Code in JavaScript by James Padolsey). However I would suggest that at a minimum:

• names of variables and methods are clear, concise, and consistent;
• each method should only do one thing;
• concerns should be well separated;
• public methods should have documentation.

Code that is submitted for review should be clean and readable. If a reviewer finds the author's code cumbersome, lumbering, or unclear, then that increases the chances of the code needing a refactor before approval is granted. If a refactor is going to be necessary to improve the code, then that is best done before a reviewer's time is consumed in reading it.

Explicit Explanation

When opening a pull request, make sure that there is a full description of the issue that the changes address, as well as a top-level overview of how the task is accomplished by these changes.

This then gives the reviewer a hook into one's intentions and allows them to more readily compare what was attempted to what was actually achieved.
This may seem almost like a no-brainer, but I have had to reject more than one PR because the effect of the code changes did not match the description of what the code actually did.

Finally, if the author has used any third party tools to audit the changes (say for accessibility) that are not part of the repository's standard checks, then this check should be listed in the pull request description so that the reviewers are informed of the standards under which the work was completed.