-
Notifications
You must be signed in to change notification settings - Fork 192
Best practises for code review
Leopold Talirz edited this page May 19, 2020
·
19 revisions
- Favor approving a PR once it definitely improves code health overall, even if it isn't perfect
- Technical facts and data overrule personal preferences
- 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
- 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)
- Review effectiveness drops substantially beyond 200 lines of code.
Don't be shy to ask developers to split PRs that require reviewing more than that.
- Does this change belong in the codebase?
- Is it integrated well?
- 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)
- Any complex lines, functions, classes that are not easy to understand?
- Over-engineering: is the code too complex for the problem at hand?
- Are there tests for new functionality?
Are bugs covered by a test that breaks if the bug resurfaces? - Are the tests correct and useful?
Do they make simple and useful assertions?
- A good name is long enough to communicate what the item does, without being so long that it becomes hard to read
- Do comments explain why code exists (rather than what it is doing)?
- Does the contribution follow generic AiiDA coding style? Prefix suggestions with "Nit:"
- Does the code change require an update of the documentation?
- PR = pull request
- CL = change list (self-contained list of changes submitted for review) = PR in our case