|
| 1 | +# Code Review Guide |
| 2 | + |
| 3 | +## Things to do before you start reviewing the PR |
| 4 | + |
| 5 | +* Make sure you are familiar with the packages the PR modifies. |
| 6 | + |
| 7 | +* Make sure you have enough continuous time to review the PR, use 300 LOC per hour to estimate. |
| 8 | + |
| 9 | +* Make sure you can follow the updates of the PR in the next few work days. |
| 10 | + |
| 11 | +* Read the description of the PR, if it's not easy to understand, ask the coder to improve it. |
| 12 | + |
| 13 | +* For a bug fix PR, if there is no test case, ask the coder to add tests. |
| 14 | + |
| 15 | +* For a performance PR, if no benchmark result is provided, ask the coder to add a benchmark result. |
| 16 | + |
| 17 | + |
| 18 | +## Things to check during the review process |
| 19 | + |
| 20 | +* Am I able to understand the purpose of each unit test? |
| 21 | + |
| 22 | +* Do unit tests actually test that the code is performing the intended functionality? |
| 23 | + |
| 24 | +* Do unit tests cover all the important code blocks and specially handled errors? |
| 25 | + |
| 26 | +* Could procedure tests be rewritten to table driven tests? |
| 27 | + |
| 28 | +* Is the code written following the style guide? |
| 29 | + |
| 30 | +* Is the same code duplicated more than twice? |
| 31 | + |
| 32 | +* Do comments exist and describe the intent of the code? |
| 33 | + |
| 34 | +* Are hacks, workarounds and temporary fixes commented? |
| 35 | + |
| 36 | +* Does this function do more than the name suggests? |
| 37 | + |
| 38 | +* Can this function's behavior be inferred by its name? |
| 39 | + |
| 40 | +* Do tests exist and are they comprehensive? |
| 41 | + |
| 42 | +* Do unit tests cover all the important code branches? |
| 43 | + |
| 44 | +* Could the test code be extracted into a table-driven test? |
| 45 | + |
| 46 | + |
| 47 | +## Things to keep in mind when you are writing a review comment |
| 48 | + |
| 49 | +* Be kind to the coder, not to the code. |
| 50 | + |
| 51 | +* Ask questions rather than make statements. |
| 52 | + |
| 53 | +* Treat people who know less than you with respect, deference, and patience. |
| 54 | + |
| 55 | +* Remember to praise when the code quality exceeds your expectation. |
| 56 | + |
| 57 | +* It isn't necessarily wrong if the coder's solution is different than yours. |
| 58 | + |
| 59 | +* Refer to the code style document when necessary. |
| 60 | + |
| 61 | + |
| 62 | +## Things to remember after you submitted the review comment |
| 63 | + |
| 64 | +* Checkout Github notification regularly to keep track of the updates of the PR. |
| 65 | + |
| 66 | +* When the PR has been updated, start another round of review or give it a LGTM. |
0 commit comments