Code review notes

Main guidelines when doing a code review

Be polite.
Don’t be condescending, no matter if you are wrong or right.
Be kind.
Explain your reasoning on why the requested change is : MUST FIX, Nit, Optional or FYI.
Data and facts are the source of true, personal opinions not.
Be clear on your explanations (this is important).
If you are not familiar with the code, attempt to run it so you commence thinking as a user of the feature and have a new look at this.
If you don’t understand a decision and is not documented on the code, ask for clarifications.

Taking too long to do a code review (1 day is long enough to block a teammate)
Being too lax (the developer always deliver good code, so you tend to extrapolate the change you are reviewing should be good as well)
Being inconsistent (this is part of being polite, if you ask for a set of changes on a specific topic you should first research exhaustively so later during the PR your decision will hold and not change as you find out more details that will invalidate your comment. This could happen but doing your homework as a review should lessen the impact of this scenario).
‘We will fix it later’ : Later never happens, if we find something that needs to be improved the time is now while the patient is open.

Copyright missing (we need a copyright header)
Code Style problems (this should be caught by the linter or a Makefile target that check on this)
Copyright updates missing (If you touch a file, the copyright date must be updated if the copyright holder is the same, if not add the new copyright)

MUST FIX : Self explanatory, must be addressed if this PR is going to be merged. You must explain your reasons, so everybody agrees with you.
Nit: A minor problem, could an indentation, not fully agree on how a code section was layout but is good anyway, this is no blocker for a PR.
Optional: Not necessarily require to be addressed in this PR, but would a good improvement opportunity for a subsequent PR.
FYI: We required to think approximately this in the future, but is not part of this PR.

Be polite (Always submit your tests results, and explain the reasoning that led you to the decisions of your implementation)
In the event that you are not sure about a change, or what motivated a previous architectural decision that is not clear. Just ask.
Explain your reasoning preferably in the code comments, so the reviewer could see your train of thought.
Don’t respond to every comment a code change counts as a response (writing fixed, done, etc…; It’s just noise)
Always assume good faith on your reviewers, they are spending their time helping you to catch any edge case that you have not considered and improving the quality of your work.

https://kelloggm.github.io/martinjkellogg.com/teaching/cs490-sp23/assets/lecture-04-code-review.pdf
https://github.com/jspahrsummers/effective-code-review/blob/main/Effective%20Code%20Review.md