Skip to Content

Wiki source code of Review Process

Last modified by Richard Kreissig on 2023/09/14 08:50
Show last authors
1 In the KIELER project we are conducting regular code reviews of (possibly) all of our sources. To make things easier, we do software assisted reviews in Crucible.
2
3 **On this Page**
4
5
6
7 {{toc/}}
8
9 = Design Reviews =
10
11 Design reviews are scheduled during our various [[project meetings>>doc:Rtsys.Meeting Notes.WebHome]]. For each review, one author and at least two reviewers (though not more than three) are selected. Design reviews take place during special design review meetings. The author's job is to prepare for the meeting by doing the following:
12
13 * Choose the classes that are to be reviewed.
14 * Prepare a meeting page in our Wiki.
15 * Fill the meeting page with all information necessary for the reviewers to prepare for the code review. That may include class or sequence diagrams. You may also want to take this opportunity to add documentation about the reviewed bits and pieces to the Wiki and link to that documentation from the meeting page.
16
17 Now the reviewers look at your meeting page and through the code that is to be reviewed. During the meeting, you will discuss the comments of the reviewers and take notes of all change decisions in the meeting notes. After the review, the author goes through all decisions and gets the changes done, noting so in the notes. The author also adds a tag to the reviewed classes to mark them as design reviewed (see below).
18
19 = Code Reviews =
20
21 Code reviews are scheduled during our various [[project meetings>>doc:Rtsys.Meeting Notes.WebHome]]. For each review, one author and at least two reviewers (though not more than three) are selected. The author's job is then to do as follows:
22
23 * Choose about four Java classes that are to be reviewed
24 * Create a Review Task in [[Jira>>url:http://rtsys.informatik.uni-kiel.de/jira||shape="rect"]]
25 * Create a review in [[Crucible>>url:http://rtsys.informatik.uni-kiel.de/fisheye/||shape="rect"]]
26 * Invite the reviewers to the review
27
28 Now it is the turn of the reviewers to perform the review:
29
30 * Read the source code of the class files in Crucible
31 * Understand the source code and the class files
32 * Write a comment if there is something that needs to be improved and mark the comment as defect
33 * If you want to put more emphasis in your opinion, create a corresponding Jira issue for the defect directly from Crucible
34 * Write a comment if there is anything that is not understood
35
36 {{info title="What goes into a code review?"}}
37 If you have never reviewed source code before, you might wonder what to write into the review. Here's a few things you might look out for:
38
39 * Is the code understandable as it is? If not, try to find out what is wrong. The problems may be that comments are missing or misleading, that the code has an awkward structure, or that it is even outright wrong.
40 * Does every class, every field and every method has a Javadoc comment?
41 * Do the field and variable names make sense? For example, loop variables are often named "i" or "j", while they should actually be called "fooArrayIndex" or something similar.
42 * Does the code contain logic bugs? For example, dead code that never gets executed or null pointer problems?
43
44 Basically, look out for anything that strikes you as odd or problematic.
45 {{/info}}
46
47 == After the Review ==
48
49 After the review it is up to the author to respond to all comments the reviewers gave.
50
51 * The author goes through all comments in Crucible and leaves a note about his decision
52 * The author may schedule a followup meeting which is only held online to review for example auxiliary classes
53 * The author adds a tag to the reviewed classes to mark them as reviewed according to the proposed rating (see below)
54
55 = Code Tags =
56
57 == Design Reviews ==
58
59 When classes are ready for a design review, they should be marked with the following tag:
60
61 {{code language="none"}}
62 @kieler.design proposed <comment> 
63 {{/code}}
64
65 After the design review has been performed, the same tag can be used to mark the reviewed classes by removing the {{code language="none"}}proposed{{/code}} modifier and adapting the comment. The comment should contain the date of the design review (//yyyy-mm-dd//) and the login names of the attendees.
66
67 == Code Reviews ==
68
69 All classes have initial code review rating //red//. The first code review lifts them to //yellow//, and the second one to //green//. The following tag marks classes that are ready for the first code review:
70
71 {{code language="none"}}
72 @kieler.rating proposed yellow <comment>
73 {{/code}}
74
75 After the code review has been performed, the same tag can be used to mark the reviewed classes by removing the {{code language="none"}}proposed{{/code}} modifier and adapting the comment. Similarly, the second code review can be marked using the {{code language="none"}}green{{/code}} modifier. The comment should contain the date of the code review (//yyyy-mm-dd//), the login names of the attendees, and the ID of the code review in Crucible (such as {{code language="none"}}KI-15{{/code}}).
76
77 == Review Overview Page ==
78
79 The design and code review tags are processed automatically using a custom [[doclet>>url:http://docs.oracle.com/javase/7/docs/technotes/guides/javadoc/doclet/overview.html||shape="rect"]] during [[Bamboo>>url:http://rtsys.informatik.uni-kiel.de/bamboo/||shape="rect"]] builds. The result is displayed in a generated HTML page:
80
81 [[http:~~/~~/rtsys.informatik.uni-kiel.de/~~~~kieler/doc/rating/>>url:http://rtsys.informatik.uni-kiel.de/%7Ekieler/doc/rating/||shape="rect"]]
82
83 == Tag Syntax ==
84
85 The complete syntax of the available tags is as follows:
86
87 {{code language="none"}}
88 @kieler.design [proposed] yyyy-mm-dd comment
89 Marks a class as design-reviewed or to be design-reviewed. For proposed classes, the
90 comment should mention the login name of the person proposing the class to be reviewed.
91 For reviewed classes, the comment should include the names of the reviewers.
92
93 @kieler.rating [proposed] <yellow|green> yyyy-mm-dd comment
94 Marks a class as code-reviewed or to be code-reviewed. For proposed classes, the
95 comment should mention the login name of the person proposing the class to be reviewed.
96 For reviewed classes, the comment should include the names of the reviewers as well as
97 the ID of the review in Crucible (for instance "KI-15").
98
99 @kieler.ignore comment
100 Marks a class as to be ignored in the code rating statistics. The comment should provide
101 an explanation of why the class is ignored.
102 {{/code}}
103
104 To be compatible with past tags, the doclet is actually quite flexible in terms of what it still accepts. However, it is good practice to follow these recommendations.