Pull Request Guidelines
When preparing for a code review, it is a great idea to enter it with a flexible mind. Pedantics and technical correctness are often blockers when one is new to reviewing code. Remember that everybody approaches programming and problem solving in different ways, so keep in mind how you word your review. We aren't stepping on each other's toes, we are learning everything together. Do not be afraid to ask questions about the code before publishing your review, especially if they are blocking your progress.
Writing a PR
When creating a pull request, ensure that the title and description are useful. Writing a good title and description will help code reviewers know better what the intention of the PR is, and give contextual clues of what to look for.
Ensure the PR has a summary as a title so that it is clear to the reviewer what is being solved at a conceptual level.
Helpful descriptions walk the reviewer through the code an put problems and concepts into a format that is easier to digest. This helps the reviewer by making it less necessary to read the codebase that is outside the scope of the PR.
Both of these things give ample context to the reviewer that can help them focus on what needs to be reviewed within just the PR.
Useful commit messages also go a very long way in helping code review. They should summarise the changes that the specific commit makes. Code commits should explain what changed and why. Explaining how is what goes into the pull request.
When useful, add screenshots. This is especially useful for code that changes things visually: shaders, UI, texture rendering, etc. as it shows the code reviewer what things are intended to look like.
You will likely be doing a lot of code reading, but a review includes testing the pull request. At minimum, you should:
- Test on all available platforms;
- Make sure to include which OS version you tested on;
- Write down any inconsistencies you find, if any.
- Check that functions have comments where appropriate.
- If the function has parameters and/or returns something, ensure they are described. Indicate the parameter names, data types passed and/or returned, if not obvious.
- Obvious functions don't need comments.
- Ensure variables are statically typed.
When to Refactor
During a code review it might become necessary or a good idea to refactor. If this happens, we should:
- Identify the refactor and confirm with others that it is needed;
- Refactor the code and test for regressions;
- Continue with the original issue.
It is important to remember that these are just guidelines and will likely not be a "one size fits all" approach. It is also good to follow what feels natural to you when reviewing code.