How to do Github peer review
I still remember the first time when I reviewed my co-worker code in a Pull Request on Github. This was extremely crazy and painful, even if the Pull Request is 200 lines of code.
- What is the code about?
- Do I understand the code fully and correctly?
- How to comment?
- Are my comments correct or not?
All questions came to my mind and blew my mind away. The task took like 30 mins and I was still confused of what I have done.
Luckily, my review skill, then, improved day by day since I had more chances to review and be reviewed by my co-workers. To me, coding helps review better and of course review helps coding better.
Reading best practices/books + reviewing good code from great developers helps the coders to learn fast and write code cleaner and more efficient as well as to be a better reviewer
Hence, I have come with a list of tips/guides/rules for reviewing
1. Understand Clean Code (book)
- This is the fundamental things all coders must know in order to deliver a good code/product as well as review better.
- This book is valuable and worth your time, its a guide for all of your coding projects.
- If a coder cannot write clean code, this means he/she is not a TRUE one.
2. Make sure the coder did review his/her own code before asking for review
- This makes sure the reviewer doesn’t waste time on trivial errors/bugs
- The coder has to consider him/her-self as the first reviewer for his effort
- This is unprofessional when sending a bad code to your boss/leader/co-worker without care
3. Make sure the owner add tests for additional methods/functions
- Without tests, functions/methods are buggy
- Adding tests (unit tests, integration tests) prevents others crew up your code
- Tests act like your certificated insurance of your code
4. Make sure you review less than 300 lines of code (added)
- The reviewer is not a machine, he/she is a limited resource. Asking a person to review more than 300 lines in his/her free time is a little rude.
- If the Pull Request is more than 300 lines, this means the coder should break it down into smaller Pull Requests in order to review faster, better, less buggy.
5. Make sure you spend around 15–30 mins for one time review
- As mentioned above, if the reviewer spends too much time on reviewing, the review performance is going to be worse than the first 30 mins.
- The reviewer is also a coder, he/she has tasks to do => then don’t ask for more than 30 mins.
6. Adopt a positive attitude
This is the most important aspect of reviewing, the reviewer must:
- respect the coder outcome
- comments politely
- show suggested solutions/fixes for bad code or show guidelines/instructions for some difficult stuffs
7. Reviewing means learning, take it seriously
- Reviewing takes time but it also helps to improve the reviewer’s coding.
1. Every Pull Request is associated with an issue, make sure you understand the issue before reviewing
- This seems a little work involved but its worth understanding the issue before reviewing its fix/solution. This helps the reviewer capture unnecessary/redundant code.
2. Start review each part of the code, make sure it works as expected with Clean Code guides
- Start with a small part of the code, review it carefully then go to others
- This helps the reviewer takes it carefully. If this Pull Request is big, then the reviewer can handle the rest for other developers to review when he/she is too busy.
3. A sample divide and conquer strategy
This is a sample checklists for reviewing
- Design and architecture
1. Single responsibility principle: One function/method should do one thing ONLY
2. Code duplication: If there is some duplicated code =>refactoring
3. Error handling: Handling error is part of coder’s tasks, make sure the functions works/fails as expected with errors
4. Efficiency: This should be the main focus on reviewing, however, I believe that Readability and Clean Code is better than Efficiency in a long run if Efficiency is implemented by hard-to-read code.
1. Method name, variable name: Accurate, meaning names
2. Function length: Less than 20 lines is perfect
3. Readability: Must be easy to understand
4. Commented code: Code is self-illustrated, no need for comments. If comment is a must for newbies then acceptable.
- Testing strategies (success/fail/exception tests)
1. Test coverage (100%): Make sure test coverage is closer to 100%, even if test coverage is 100%, it does not guarantee the project won’t fail.
2. Test side effect: Test exceptions, invalid inputs
3. Test right level of permissions/authorisations: Make sure the coder writes tests with correct permissions/authorisations for particular tasks/apis/views.