First of all: perform a self-review, that is read your code, before you ask somebody else to read it. You might verify items from the checklist below:
- are there any TODOs left in the code?
- is the documentation up-to-date with the changes? will a new user be able to understand how to use the changed code?
- do public methods have comments?
- are comments & documentation free of typos? (the IDE should help with that)
- will the comments & documentation be understandable to a new person, not familiar with the project (or at least to somebody, who didn’t spend the last week implementing the solution )
- are there any complex places in the code, which would need explaining? if so, a comment would be probably useful
- does our implementation require describing it in the PR? if so, it’s probably better to add a code comment instead
- is the code formatted?
- are there tests? in case of a bug, are there tests reproducing the bug, which failed without the changes? in case of a feature, are there tests testing the new functionality?
- are there any new warnings when compiling?
- did CI pass?
Additionally:
- link the issue, which a given PR closes (e.g.
Closes #123
) - that way, the issue will be automatically closed after merging. Assign yourself to the issue, so that it’s clear that somebody is working on it - when changing the UI: add a screenshot to the PR, demonstrating the effect of the changes
- the name of the PR should include something more than the issue number, a very short description of what it is about will suffice
- when there’s a discussion on the PR, consider a refactor or a code comment. A discussion means that something in the code isn’t clear - it might also not be clear to any future reader of the code, who didn’t participate in the PR.
The above doesn’t mean that the PR should be created only once the code is polished & fully “done”. It’s a good practice to publish your work on an ongoing basis (once daily minimum). PRs which are “work in progres” should be marked as “draft”, which sets the right expectations.