HàPhan 河

Code Review - a checklist to do it right

Overall: Look for Improvement - Do Encouragement - Share best practices - Not to hold the PR - Minor things should be noted but tell developer it's skippable.

  1. Design:
    Does the code change make senses? is it a good time to add? how does it integrate with our current code base or system?

  2. Functionality
    Determine the users of this code change: both end-users (feature impact) and developer (future use, future enhancement?)
    Ask for a demo/UI if need to understand the reason
    Introduce a race condition or deadlocks?

  3. Complexity
    Functions are too complex?
    Classes are too heavy?
    “Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.”
    Over-engineering: code is more generic than it needs to be, solving a future problem? future problems should be solved when it arrives not the time of PR.

  4. Test
    Ask for Unit tests, integration tests, end-to-end tests
    Test should never test itself
    Tests are code so that they are need to be easily maintained

  5. Naming
    Naming is hard, should pick a good name: descriptive and describe correctly the purpose

    Comment WHY not WHAT to do
    Check the necessary of the comments
    Need to be a clear and understandable ENGLISH (don't use Japanese)
    Comments are different from documentation

  7. Styles
    Follow Swift Lint rules https://realm.github.io/SwiftLint/rule-directory.html
    Run SonarQube before making the PR, always check SonarQube results after PR made

  8. Documentation
    Should always be updated with the code
    Delete if code is deleted
    Update README in case new code violates it

  9. Every line
    It's a must to go through every line of code that have been assigned to you.
    Some codes are auto generated, but never skip human-written code.
    For large code base, break into parts to review, note in comment for the part that reviewed or mark reviewed using Gitlab interface then can continue at another time.
    Ask for clarification if any codes slow down your review.

  10. Context
    Sometimes need to have a broader context to tell code changes make sense: for example several lines are changes, but need to read whole class to decide correctly.
    Don’t accept PRs that degrade the code health of the system

  11. Good thing
    If see something nice in the PR, tell the developer. Code reviews often emphasize on mistakes, but should offer encouragement and appreciation for good practices.

Based on https://google.github.io/eng-practices/review/reviewer/looking-for.html