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.
- 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? - 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? - 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. - 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 - Naming
Naming is hard, should pick a good name: descriptive and describe correctly the purpose - Comments
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 - 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 - Documentation
Should always be updated with the code
Delete if code is deleted
Update README in case new code violates it - 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. - 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 - 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