Cheetah - SKA - PSS - Prototype Time Domain Search Pipeline
doc/code_review_guidelines.md
1 \anchor code\_review\_guide
2 # Code Review #
3 
4 The review process is an analysis of the code to help improve code quality.
5 A good review will not only catch the usual slips/problems of style and bugs, but
6 should also consider the overall health of the project and how any new code fits in.
7 Always be on the lookout for opportunities to refactor and reuse as well as
8 highlighting any flaws in our assumptions in the whole architecture.
9 
10 Consider the following points when code reviewing wherever possible:
11 
12 ## High level overview
13 - Verify that the feature branch is synced with the latest version of dev
14 - Check the files that have been changed make sense? Are files present that shouldn't be there (e.g. build products, dot files from some toos etc)?
15  Note this guide assumes you have an up to date local copy of the dev branch chacked out as "dev". If you prefer you can specify "origin/dev" for the branch name)
16 ```
17  git diff dev --name-only
18  ```
19 - Have any thirdparty dependencies been changed? They must reflect the repositories they are drawn from and cannot be changed arbirarily.
20  Each dependecy will have its own rules. See the thridparty/README for more details.
21 - scan through the logs - are the commit messages clear?
22 ```
23  git log
24 
25 - check for whitespace errors
26 
27  git diff dev --check
28 ```
29 - scan through the actual changes
30 ```
31  git diff dev .
32 ```
33 ## Compilation
34 - does it compile in your environment?
35 - does it compile in debug and release modes?
36 - does it compile on the reference environment (check skabuildmaster)?
37 - where the new code requires an optional external dependency, does it compile when this is turned off (i.e. with cmake -DENABLE\_<dep>=false)?
38 - ensure there are no new warning messages in the compilation - all warning should be treated as errors without an explicit exception
39 
40 
41 ## Unit Tests
42 - Is there a unit test for each class and the test compiles?
43 - If there is no code to test is there adequate documentation to explain in the unitTest header file
44 - Do the tests conform to the style guidelines?
45 - What is the coverage?
46 - Have all the major use cases and corner cases been considered?
47 - Is there any scope for genralising components of the test into the test\_utils library?
48 - Could the tests use existing components inside the test\_utils library?
49 - Do the unit tests all pass (in debug and release modes)?
50 - Could the code being tested be simplified (design/interfaces etc) to improve testing?
51 - Does valgrind report any errors running the unit tests (i.e verify there are no memory leaks)?
52 
53 ## Looking over the code
54 - does the code conform to our style guidelines
55  - adequate documentaion
56  - indentation
57  - poor or dangerous constructs
58  - all redundant includes removed?
59  - includes correctly placed in header or cpp file?
60  - is the header file readable, in our standard format and are there adequate comments in the code
61  - variable, method and class names conform
62 
63 - look out for potential problems
64  - is the design modular and scalable? Does it make sense to refactor this?
65  - can you see any obvious bugs? Why was this not caught by the unit test?
66 
67 - Packaging
68  - is this code in the right place? Should it be broken out in to its own module or merged with a different one?
69  - check for circular dependencies between subpackages