| | 1 | Both implementor and reviewer must be very careful. |
| | 2 | Please check that you understand all of the above. |
| | 3 | |
| | 4 | For each code task (no matter whether it is a library, external feature, bug fix...), should pass the following check list: |
| | 5 | |
| | 6 | * code |
| | 7 | * The code style is correct to our convention. |
| | 8 | * The code is easily readable. |
| | 9 | * There are no fake JavaDocs (JavaDocs without useful information). |
| | 10 | * The java-doc is complete enough. |
| | 11 | * For each element, the JavaDoc should describe clearly what is it (if this is not easy, then probably the design is bad) |
| | 12 | * It causes '''0''' errors. |
| | 13 | * It causes '''0''' warnings. |
| | 14 | * The @SuppressWarnings annotation is used only if really needed. |
| | 15 | * I know 3 cases for it - |
| | 16 | * unused, when something is used only by reflection |
| | 17 | * synthetic-access, when you want to touch something internal with inner class (although in many cases this may be avoided) |
| | 18 | * unchecked, when you are doing something complex with generic (note that this is also avoidable in many cases) |
| | 19 | * No inaccurate, or unclear XXX and TODO tags |
| | 20 | * It should not break other code (ensured by their tests). |
| | 21 | * other suggestions? |
| | 22 | |
| | 23 | |
| | 24 | * design |
| | 25 | * It is documented |
| | 26 | * (not only in the code). |
| | 27 | * It is simple and understandable. |
| | 28 | * It is easy to use. |
| | 29 | * (using it requires less work / small code) |
| | 30 | * It is error proof (hard to use it in a wrong way) |
| | 31 | * The compiler prevents most errors. |
| | 32 | * (it is better to have an abstract getKind() instead of forcing implementors to call setKind at construction) |
| | 33 | * ('''final''' may also help) |
| | 34 | * Most of other errors cause an exception |
| | 35 | * (defensive programming helps here) |
| | 36 | * When a library is used, it is used correctly. |
| | 37 | * There are no representation exposures. |
| | 38 | * There are no GOD (very long) methods. |
| | 39 | * There are no GOD (very long) classes. |
| | 40 | * The coupling is low. |
| | 41 | * No magic numbers (and strings) |
| | 42 | * No array members. |
| | 43 | * (unless there is really a reason) |
| | 44 | * No public instance fields. |
| | 45 | * No public non final fields |
| | 46 | * It has high cohesion. |
| | 47 | * (see http://en.wikipedia.org/wiki/Cohesion_(computer_science) ) |
| | 48 | * It has low coupling. |
| | 49 | * (see http://en.wikipedia.org/wiki/Coupling_%28computer_science%29 ) |
| | 50 | * It is possible to state in one sentence what every class or member is/does. |
| | 51 | * Classes and members are named correctly. |
| | 52 | * Every class (or attribute) is self responsible and self complete. |
| | 53 | * No code duplication (copy paste). |
| | 54 | * It is easily testable. |
| | 55 | * (when not, testing helpers are provided to make it such). |
| | 56 | * No other kind of smells. |
| | 57 | * It has good package structure. |
| | 58 | * It is logging correctly. |
| | 59 | * It has adequate error handling. |
| | 60 | * No premature optimization. |
| | 61 | * No premature abstraction. |
| | 62 | |
| | 63 | * tests |
| | 64 | * It has automatic use case tests, '''before''' implementing. |
| | 65 | * (they are very good to demonstrate the design for a library) |
| | 66 | * If it has demos, the demos are actually tests with main method. |
| | 67 | * (demos should be done at design time!) |
| | 68 | * It has enough unit tests to ensure its correctness |
| | 69 | * (listed at design time, passing at implementation time) |
| | 70 | * It has enough integration tests |
| | 71 | * (listed at design time, passing at implementation time) |
| | 72 | * It has enough system tests |
| | 73 | * (listed at design time, passing at implementation time) |
| | 74 | * It has written scenarios for manual testing |
| | 75 | * (when it is an external feature or a bug) |
| | 76 | * The tests had code coverage |
| | 77 | * (good code coverage does not mean good tests, but bad code coverage means bad tests) |
| | 78 | |
| | 79 | * other: |
| | 80 | * Enough information should be provided in the backlog. |
