code-review.md

Code reviews

• Why do them?
• What to look out for

Code Review, or Peer Code Review, is the act of having your code checked by others for mistakes, errors, and omissions (e.g. missing tests). Like pair-programming (which we also support), it accelerates and streamlines software development.

While we can support "traditional" Code Reviews, we routinely review via pull requests. These are communicated in the #frontend-pr Slack channel, and managed via GitHub. Discussion should happen in the GitHub comments, not Slack.

Requesters merge their own changes when they're ready. Keep your fingers off that tempting green merge button.

Why do them?

PR reviews are cross-team, thereby helping to foster discussion and feedback

1. Knowledge sharing and transfer within and between teams:
   • Allows everyone to learn from their colleagues.
   • Allows greater visibility and familiarity of other team's work.
   • Helps prevent knowledge silos or domain-specific codebases.
   • Allows FEDs to share expertise.
2. Standards compliance across teams:
   • Aids the ability for developers to easily move between teams.
   • Ensures changes to codebase conforms to the same standards and guidelines.
   • Helps identify areas where code re-use are beneficial ("should that be a component?").
   • Helps identify areas of compliance, which are missed during development.
   • Fosters caution and code quality over temporary hacks.
3. Prepares people for work on open source software projects.
4. Generally aids onboarding and training.

What to look out for

• Accessibility

  • Accessibility problems. Suggest testing with pa11y and/or other tools.
    • pa11y dashboard (link only works if inside the Springer Nature VPN)

• Complexity

  • Overly complex code: code that solves problems we don't have, or makes things more complex instead of less.
  • Code that introduces out-of-date or unnecessary dependencies into the project.
  • Changes which may have wider ramifications than the author anticipated. Double check they are aware which can of worms they have just opened :)

• Performance

  • Premature and/or micro optimisations.
  • Non-performant code. Suggest testing with Lighthouse, SpeedCurve, WebPageTest and/or other tools.

• Scope

  • PRs which are non-atomic and should be split into separate PRs, e.g. a new UI feature combined with an unrelated configuration change.
  • Code which could be split up into reusable modules.

• Security

  • Code that adds dependencies that contain known vulnerabilities. Suggest testing with tools like npm audit, Snyk (for open source repos) and/or White Source (for private repos).

• Syntax

  • Syntactic inconsistencies the linter did not catch.