CodeReview.md

Code review policy

🍉 Basics

🚓 Policy

• Review over codding! Review PR within "n" hours after its creation. Makes PR owners stay in the context of their work.

🗺️ Reviewing strategies

  Per file

   Overview
   Review a PR file by file. Can be performed in Github UI. Suits small PRs.

   Advantages

• Time efficient

   Disadvantages

• Gives poor task context
• Risk to miss errors

  

Per commit

   Overview
   Review by commit history

   Advantages

• Gives more context to task
• Requires not much time

   Disadvantages

• Requires clear commit history

  

Check out locally

   Overview
   Pull code and review it locally. git merge --no-commit --no-ff

   Advantages

• Gives the most context possible
• Allows to test your suggestions
• Allows to run code
• Supports referencing (easier code navigation)
• The whole file is visible against the changes area in Github UI

   Disadvantages

• Time-consuming
• Effort-consuming
• Requires to stash changes to your current task

🗒️ Checklist

• verify CI checks passed
• optional: indicate review in-progress
• read ticket
• optional: read commits history
• optional: go through others’ comments if any
• optional: merge branch locally
• validate cross-module dependencies build.gradle.kts

 while reviewing consider the following:

    ⬜ tests provided
    ⬜ deprecated code removed
    ⬜ syntax & formatting is correct
    ⬜ can anything be simplified?
    ⬜ security flaws or potential holes
    ⬜ are the naming conventions appropriate?
    ⬜ is there documentation needed?
    ⬜ architecture
    ⬜ object-oriented design principles
    ⬜ reusability
    ⬜ performance
    ⬜ manageability (readability)

• review “standalone” files and their tests (e.g. domain models, API services, dagger)
• review coupled files and their tests (e.g. presenters, views, interactors)

💡 Tips

• Remove old local branches git remote prune in order to find the target branch faster locally

🤔 Points for consideration

• Merge condition
• PRs priority (high/medium/low priority). Higher priority PR should be reviewed first.
• PR impact (high/medium/low). Low-impact standalone PRs could be merged with less number of reviews.
• Prioritize merging PRs over developing new features (potential merge conflicts)
• Deadlines for reviewing PR’s - allows decrease a number of re-reviews of every single reviewer. Split review process into iterations: “review” → “addressing comments” → “re-review” → “addressing comments” Aim: avoid frequently switching contexts, reduce number of re-reviews PR statuses
• Reviews limit (e.g. no more than 3 reviews including code owners)
• High-level plan of merging all of the PRs
• How to deal with legacy, should we review it?