Skip to content

Best practises for code review

Leopold Talirz edited this page Mar 12, 2023 · 19 revisions

Standards

Approving changes

  • Technical facts and data overrule personal preferences
  • Favor approving a PR once it definitely improves code health overall, even if it isn't perfect

Vigilance

  • Be responsive to review requests.
    AiiDA users who put in the effort of contributing back deserve our attention the most, and timely review of PRs is a big motivator.
  • Look at every line of code that is being modified
  • Use a checklist like the one below, especially if you are new to code review

Communication (more great advice)

  • Offer encouragement for things done well, don't just point out mistakes
  • Fine to mention what could be improved but is not mandatory (prefix such comments with "Nit:" for "nitpick")
  • Good to share knowledge that helps the submitter improve their understanding of the code (clarify where you do/don't expect action)

Checklist - What to look for

Scope

  • Are you being asked to review more than 200 lines of code?
    Then don't be shy to ask the submitter to split the PR - review effectiveness drops substantially beyond 200 lines of code.
  • Are there parts of the codebase that have not been modified, but should be adapted to the changes?
    Does the code change require an update of the documentation?

Commits

  • Does the PR consist of self-consistent commits that each tackle a well-defined problem?
    If reading the commit messages still leaves you wondering what the commits do, the commit messages should probably be improved!
  • Do the commit messages follow the guidelines?

Design

  • Does this change belong in the codebase?
  • Is it integrated well?

Functionality

  • Does the code do what the developer intended?
  • Are there edge cases, where it could break?
  • For UI changes: give it a try yourself! (difficult to grasp from reading code)

Complexity

  • Any complex lines, functions, classes that are not easy to understand?
  • Over-engineering: is the code too complex for the problem at hand?

Tests

  • Are there tests for new functionality?
    For bug fixes: is there a test that breaks if the bug resurfaces?
  • Are the tests correct, useful, and easy to understand?

Naming

  • Good name: long enough to communicate what the item does, without being so long that it becomes hard to read

Comments

  • Do comments explain why code exists (rather than what it is doing)?

Style & Consistency

  • Does the contribution follow generic AiiDA coding style (mostly enforced automatically)?
    Prefix suggestions with "Nit:"

Glossary

  • PR = pull request

Sources