Code Review guidelines (WIP)

When code reviewing a piece of work, the reviewer’s task is to ensure that the code:

• Follows “good” practices

• Will likely work as intended

• Will not break other functionality
  
  

In order to achieve this, there are a few different things the reviewer will bear in mind.

Generic Guidelines

Reviewer/Reviewee

In general, a review will be carried out by a developer who has more experience (either in the framework, module, or both) than the developer who carried out the work. However this won’t necessarily always be the case, and the context of who did the work and who is reviewing can be useful for gauging the best way for both reviewer and developer to take something from the interaction.

Context

Understanding the context of a code change is important. A change in isolation may seem like a good one, swapping out a destructured prop name from thisIsAPorp to thisIsAProp everywhere it’s used within the module. However if that module is a shared library such as stripes-erm-components, then it’s possible that the property is in use elsewhere, and it represents a breaking change.

This is just an example, but the layers of context can change what seems like a perfectly reasonable change into one that causes issues. The layers in particular:

• Framework - is it react, grails, inside or outside FOLIO?

• Module - is the change in mod-agreements or mod-serials-interaction, does that make a difference to code practices?

• Location - Are the changes in the correct place? Should this be centralised or localised?

• Function - Code for displaying something to a user will need different attention to code fetching something from the backend, for example.

• Code - Final layer of context is the code itself. Are there any antipatterns, does the logic make sense at a glance? Are comments in place?

Testing

There are guidelines in place around FOLIO that a testing threshold of 80% is expected. This is in placfe to try and ensure a good and responsible test coverage across all modules, but is often substituted for the same. In reality it is up to the developer to ensure that tests are in place for any functionality, or bug fix to ensure that changes elsewhere would not impact the component or class at hand.

From a review standpoint, the main criteria are whether or not tests exist and are passing, but a cursory glance can help identify cases where a developer has potentially missed something important that ought to be tested.

• Are tests needed?

  • If a change has been made to code, that probably should break tests.

  • If a change has happened but no tests are changed or added, either the fix/feature is not tested – which should be asked of the developer, or the tests in place do not actually test the functionality as they should - which should again go back to the developer.

• What sort of tests are required?

  • Changes in frontend will require cypress tests and/or jest tests

  • Jest tests should do more than just check the interior stripes components render and work as expected (filling out fields etc)

    • In general, if a component “does something”, like check if one field is filled to give a warning on another field, that should be tested

  • Cypress tests should set up and then delete all the data needed for them to run, so they can run on any system without clogging it up entirely (Still don’t run against live production builds)

Communication

Developers can obviously be contacted both directly via the PR/JIRA ticket or through other means such as Slack etc. In general, a problem and worked through solution should be trackable in future, so it is important that at the very least a record of decision making and questions/answers is available somewhere. The Jira ticket and/or the PR are probably the best places for those, with any particularly complicated or “weird” decisions possibly warranting a comment in the code itself for the next developer.

On that note, code legibility is a huge part of review. The “cleverer” a piece of code is, the more likely it is to cause massive headaches to a different developer, or even the same one months down the line. A reviewer should be approaching the review asking not “do I understand what this code is doing”, but “could everyone understand what this code is doing”.

A general rule of thumb is that anything that takes the reviewer at least an extra beat to figure out likely needs at the very least a comment. Really complicated array reductions or chains of logic, etc are going to cause problems for the next person, and possibly introduce weird interactions that are hard to foresee. These should be at the bare minimum commented with the approach, but sometimes it’s better to rework entirely.

Sometimes there will be code that the reviewer does not understand, or decisions made that do not align with the reviewer’s approach or line of thinking. The review is not necessarily about shaping all code to match the same approach. However, these differences in style can lead to gaps in logic through which bugs can fall.

When confronted with uncommented code that appears to be a strange decision or one that differs significantly from the reviewer’s style, it is usually helpful to reach out and ask why the developer took a certain approach, or ignored a more obvious solution. Sometimes the developer may not have thought about the other possibility, sometimes it may reveal that they did try it the “obvious” way, and that led to unexpected bugs.

In those cases, that may be an indication that something more “weird” is happening under the hood, and the code suggested might work, but be covering up that strange behaviour which should be weeded out first. In other cases, there are legitimate reasons why a developer ruled out particular paths of logic. In that case there should be a documented comment thread (see above) and a comment in the code explaining why the approach was taken (because if you asked the question so will another developer).

In a similar fashion, if code in one place is known by the reviewer to match or have impacts elsewhere, a comment should be made on the PR to check the other places. This should indicate that a comment is needed in the code to point the developer at those hidden dependencies, and tests ought to catch and fail at situations like this.

Time crunch

Certain decisions can be made in context that would otherwise be advised against. Tests can be skipped, slightly wonky code can be allowed through, solutions that should be centralised can be left local.
If up against tight time or resourcing constraints, sometimes it’s the right thing to do to allow these things through the net. However it is then up to the reviewer to raise them with the team at meetings and ensure that follow up work to recover the tech debt is not lost.

Module Specific guidelines

TBC - ui-serials has centralised testing etc etc