When you are asked to be a reviewer in a Pull Request, you become accountable for. That mean, after the code will be merged to the main stream, your name will be there, so take it seriously.
In this post, I would like to list various points I try to follow when I’m marked as a reviewer in a pull-request.
Usually we also mark the associated ticket number, and status in the PR title, therefore we can easily identify what it will be about.
Is the feature working? Is the bug fixed?
The first thing I do usually, is to checkout the branch, and try the feature or the bug fix. For me, this is the most important thing for now.
- I check if all points of the requirements are matching.
- I try the steps to reproduce the bug to make sure the issue is fixed.
- If the code touches some shared part with other feature, I try that as well to make sure it won’t cause any regression
- If the code is touching the UI part, I also try on various devices. lower iOS, iPad, bigger screen phones… rotation.. To make sure the UI looks good and matches the initial design.
- Running the unit tests.
If something is not matching, I’m sending back the PR.
If the feature works, I then check the architecture decision, and how the code is arranged all together.
- Usage of shared code. We should leverage the code we have and not rewrite something which exists
- Usage of SOLID principle?
- Are we able to add unit tests for the code?
- Are we able to make a generic code, which could be extracted as a library?
- Using dependency Injection, where are the instantiations done?
- Are we able to clean legacy code
Is the code following some of our coding guidelines
- Small function, small classes
- Variable names are understandable and clean
- Following Swift standard lint and rules
- Coding style (header, spacing, brackets alignement…)
Are you following any rules when doing PR review?
Thank you for reading until here… 👊 Have a good week end… 🍺