6.5. Exercises

Exercise 6.1

Define a code review process for a small development team with an approximately equal mix of experienced and inexperienced software developers. Follow the process template.

Background for Solution:

Here is one possible solution for a code review for a small development team.

Small Corp Software Inspection Process

Process Rationale:

There are several issues in the current software development process which could be mitigated by regular code reviews. Currently, the software is sometimes unable to satisfy all use cases or provide all of the functionality its interface would suggest. There has been a lack of reuse of client access code, which creates several unnecessary maintenance points for code of sometimes significant complexity. The actual naming and coding standards differ from the corporate coding standards. Frequently, the software contains several defects which are not discovered until integration or, in some cases, deployment. Additionally, maintaining code has been difficult when people leave due to a lack of comments and esoteric coding practices. For example, people introduce third-party libraries to solve a particular problem without telling anyone or even documenting the vendor source of the library. Regular, systematic code reviews are expected to significantly reduce such incidences.

Process Goals:

The software inspection process has three main goals. The first is to improve software quality. What is meant by "improve software quality?" improve in relation to what baseline? The baseline is a process with the same timeline, but no inspection process. Thus, the baseline consists of a process with a small amount of additional time for all the other development activities. We expect to improve quality by discovering and correcting inadequate comments, poor design, and errors early in the software life cycle. The second goal is to create and disseminate recommended code solutions for solving specific, recurring technical problems. The third goal is to facilitate maintenance and enhancements by increasing uniformity of software styles.

Process Outline:

The software inspection process has three main phases: before the software inspection meeting, during the meeting, and after the meeting.

Before the Meeting:

The inspection process is based on three-person round-robin teams. Figure 6.3 illustrates the concept. Teams can be self-appointed or assigned, but everyone with new code should be on a team, and teams should change membership from one round to the next. Each team member chooses one or more major classes to be inspected and distributes them to each member of the team. Each member reviews the code written by one other member before the meeting starts and brings the annotated code to the meeting for discussion.

A software inspector should look for the following kinds of things. The most obvious are errors such as failing to take account of all cases, dividing by zero, failing to check array bounds, failing to check for null pointers, etc.

Also, make sure that the basic purpose of a class is documented and that complex code, e.g., code involving third-party software such as Objectstore, is explained in comments.

Still another type of comment is just checking that all requirements are met and that the code matches the documented design.

As for software style standards, e.g., indentation and naming, Eidea Labs has adopted Scott Ambler's Java coding standards. All inspectors should be familiar with these standards.

See http://www.ambysoft.com/javaCodingStandards.html for more information.

During the Meeting:

Here is an example of how three-person round-robin inspection should work. Suppose that Vick, Christy, and Zhian are on a team. First, the team decides who will review whom. Each reviewee selects some new code that needs review and tells the reviewer where it is. For example, Zhian reviews Vick's code, Vick reviews Christy's code, and Christy reviews Zhian's code. Each team member brings three copies of reviewed code with annotations including suggested improvements and highlighting of possible problems.

The flow of the meeting will look something like this:

∼40-minute review of Vick's code

Break

∼40-minute review of Christy's code

Break

∼40-minute review of Zhian's code

Let's keep the meetings from taking forever, and let's prevent review of one person's code from taking up all of the time allocated for the remaining reviews. If time runs out, time runs out. Move on to the next person's code. Take the rest offline.

During the meeting, a reviewer hands out the annotated code. The reviewee narrates his/her code by stepping through one thread of execution in methods of a class, or through an important thread in client code. Both of the other participants ask questions and give suggestions. The reviewer makes sure that all his/her comments are covered. The third person needs to watch the clock! The third person should help to keep the meeting on track, try to resolve disputes, and take notes of cool techniques and sneaky bugs. The group should reach consensus on changes that need to be made to code under review, and the reviewee should record his/her own action items for those changes.

After the Meeting:

The participants of an inspection meeting are expected to produce several deliverables. Of these, software revisions according to the meeting recommendations are the most important. In addition, one person from the review documents the meeting with a note containing at least the following information:

• Line of code reviewed

• How long the meeting lasted

• Number of errors identified (no author data; missing comments not counted)

• Recommended solutions to recurring problems (with code samples as appropriate)

A physical file with a copy of all annotated code and the meeting document will be created. The files will be organized by date and the names of all participants. These files will serve two purposes. First, they can be used to verify that participants complete meeting action items. Second, they can be used to establish a longitudinal record for evaluating the benefits of the software inspection process itself.

Process Deliverables:

This section provides a simple checklist of all the deliverables produced during the process:

• Annotated code (annotations from before and after the meeting) from each participant

• Corrected code from each participant

• Meeting document

• Folder containing all of the above

Process Timetables

Single Software Inspection Timetable:

The timetable for one round of a software inspection process is as follows. Choose a team and pass out code. Allow three or four days between everyone's receiving the code and the meeting. The meeting should take about two hours, as mentioned above. Fixes recommended during the review should be performed immediately. The amount of time required for these fixes depends greatly on their nature. Redesign might take a week, while adding comments may take only an hour. The results note should be written and sent by close of business (COB) the following day.

Software Inspection Cycle:

From a longer-term perspective, software inspections should take place every two weeks during the implementation phase of the life cycle. After each inspection, each participant will require approximately one day to make changes according to action items and to create the meeting document.

Software Inspection in the Software Development Life Cycle:

Where in the overall software development life cycle should the inspection occur? The code should be inspected during unit test and before integration. Once code from multiple authors has been integrated, it is far more labor intensive to isolate and correct errors. By inspecting the code before this phase, we can eliminate many time-consuming integration-phase problems.

References:

Abstracts of several papers on software inspection are available at these URLs:

http://www/ics.hawaii.edu/~johnson/FTR/Bib/category-metrics.html

http://www.ics.hawaii.edu/~johnson/FTR/Bib/urls.html

Software Architecture Tip: Practical MVC Use In Basic Training you were introduced to the MVC and Observer design patterns. Many of the constructs in the Java language require the implementation of one or more interfaces to use preexisting view components with a particular data model. So what do you do when you have several different views, each of which demands a particular data model interface implementation? See Figure 6.4 for one approach. Figure 6.4. One Approach to Coordinating Multiple Interfaces and Data Models Do not create a "manager" class to coordinate the disparate models. Such an approach is unwieldy and error-prone. Each data model would possess its own copy of the data, and extensive code would be needed to keep the models in sync. In addition, adding in a new representation would require changing existing methods in order to accommodate the new data model and its classes. Instead, consider the motivation behind the MVC design pattern: "to have a single model which supports multiple views. The view should know about the model but not the other views." With that in mind, Figure 6.5 presents a cleaner, more effective approach. Figure 6.5. Integrating Several Data Models and Views Effectively Here you have a single model which controls all the data needed by the various views. The model has the responsibility of implementing all required interfaces so that they all use the single data set. Each view communicates with the single data model implementation via its expected interface. Each inherited interface serves as a role for the data contained by the data model. While implementing such a large number of interfaces appears unwieldy, it is an overall better architecture, as it lends itself well to future extension and reduces the overall design complexity.

Exercise 6.2 Teamwork Exercises:

For each of the following two scenarios, write a brief analysis of what is motivating the behavior of each character type and what actions can be taken to resolve the situation. Both stereotypes are on a software development team of six people, including an architect and project manager.

"Lone Wolf Developer": Team member has little patience for the overall team goals. Instead he/she insists on being given a well-defined piece to develop in isolation. On previous tasks, he or she has tended to disregard agreed-upon interfaces and has been extremely reluctant to provide detailed designs prior to beginning implementation. Whatever is provided before implementation is always in a state of flux, with most of the documentation being delivered after the implementation is complete.

"Unilateral Consensus": Team member's opinion dominates the group to the point where there are never any dissenting opinions. The team member has technical superiority and a forceful personality. When his/her opinion on an issue is known, other options are no longer considered.

Background for Solution:

"Lone-Wolf Developer": This person appears to lack a buy-in into the common purpose of the team. His/her actions indicate a shirking of responsibility to the team and no belief in mutual accountability. There is a feeling that success on his or her part is separable from the overall success of the team. Allowing the "lone wolf" to continue destroys the mutual trust between him/her and other team members. Infighting is inevitable when interfaces change without warning, vital documentation is missing, and integration is needlessly delayed by a continually changing codebase.

Regular design and code reviews are essential in ensuring that some communication exists between lone wolves and the rest of the team. By reviewing work products that they have a personal vested interest in, obtaining their attention is assured, and hopefully, constructive "forced" interaction will lead to more regular informal interactions in the future. From the management perspective, the lone-wolf tasks should be scheduled in small increments, ideally no more than three days in length. After each task, they are responsible for documenting their work and integrating with other people who, ideally, work closer to the baseline. Finally, make it clear that proactive communication with team members is the primary determinant of future responsibility. Specifically, an individual should neither spend an inordinate amount of time stuck on a particular problem, nor allow anyone else to spend time on problems whose solution is already known within the team.

"Unilateral Consensus": First, talk to the expert and make it clear how much his or her expertise is valued and considered an asset to the organization. Next, when he or she suggests an idea, ask for the general heuristics which reinforce why the particular approach is superior. Have either the expert or other team members document and consolidate the design heuristics provided. Over time, the heuristics will demystify the decision-making process of the expert and allow team members to reintroduce relevant heuristics in future discussions with the confidence that their suggested approach is at least valid in some circumstances.

Next, change the way meetings are conducted. Create a rule that the opinions of the less-experienced team members are heard first. This results in two desirable outcomes. Less-experienced team members are given a chance to establish ownership of some ideas which will later be validated by the expert and more experienced team members. Or, and admittedly more commonly, the more experienced members are forced to explain the flaws in the suggested approach of less-experienced team members, which helps them learn by starting the discussion in areas they understand rather than the "foreign ground" of the expert. Also, by getting their ideas out first, they are more likely to defend them, which is better than not having an opportunity to suggest them at all.