5 Reviews
Anything becomes interesting if you look at it long enough.
—Gustave Flaubert
The fifth chapter of the Advanced syllabus is concerned with reviews. As you will recall from the Foundation syllabus, reviews are a form of static testing where people, rather than tools, analyze the project or one of the project’s work products, such as a requirements specification, design specification, database schema, or code. The primary goal is typically to find defects in that work product before it serves as a basis for further project activity, though other goals can also apply.
In the Advanced Technical Test Analyst syllabus, we focus on reviews of technical work products. Chapter 5 of the Advanced Technical Test Analyst syllabus has two sections.
1. Introduction
2. Using Checklists in Reviews
Let’s look at each section.
5.1 Introduction
Learning objectives
TTA 5.1.1 (K2) Explain why review preparation is important for the technical test analyst.
Again, think back to the beginning of Chapter 2. We introduced a taxonomy for tests, shown in Figure 2–1. You should remember, from the Foundation syllabus, the distinction between static and dynamic tests. Static tests are those tests that do not involve execution of the test object. Dynamic tests do involve execution of the test object. In Chapters 2 and 4, we talked about test techniques and quality characteristics, mostly from the point of view of dynamic testing.
Figure 5–1 Advanced syllabus testing techniques
In this chapter, we cover static testing, focusing on one branch of the static test tree, that of reviews. (We covered static analysis earlier, in Chapter 3.) The material in this chapter builds on what was covered on reviews in the Foundation syllabus.
To have success with reviews, an organization must invest in and ensure good planning, preparation, participation, and follow-up. It’s more than a room full of people reading a document.
Good testers make excellent reviewers. Good testers have curious minds and a willingness to ask skeptical questions (referred to as professional pessimism in the Foundation syllabus). That outlook makes them useful in a review, though they have to remain aware of the need to contribute in a positive way.
This is true not only of testers but of everyone involved. Being a group activity, all review participants must commit to well-conducted reviews. One negative, confrontational, or belligerent participant can damage the entire process profoundly.
One important part of ensuring well-conducted reviews, with proper attitudes and behaviors, is ensuring that participants have been trained. Common courtesy alone, while helpful, is not enough; in fact, excessive courtesy can stymie reviews. Each reviewer must know the right ways to discover and communicate defects, questions, and comments. People are not born knowing how to participate effectively in reviews. How to use a checklist of the kinds of defects to look for, and in which documents, for example, is not obvious (nor are the checklists themselves obvious), but people can learn to do so.1 How to participate effectively in brainstorming solutions in a technical review is also not obvious and also something people can learn. How to rephrase a potentially harsh or confrontational—but accurate—observation about a defect into a constructive, collaborative—and equally accurate—observation is something people can learn to do. Reviews, like any other form of testing, are not like breathing: You weren’t born knowing how to review and test software, and for some people it comes a lot less naturally than for others.
As a technical test analyst, in addition to professional pessimism, you should bring your technical expertise to bear on the question of, What is likely to happen in operational or production environments if this design, this architecture, or this code is put into use? That’s a different question than one of elegance or even correctness, because it focuses on suitability of the behavior. Sometimes, you might find that checklists for work products on design, architecture, and code don’t always consider this perspective, so be ready to help define, apply, and maintain checklists in order to reflect such a perspective. (We’ll discuss checklists further in the next section.) In addition, when reviews reveal defects, you should help participants understand why these defects may have a higher impact on users, customers, and business stakeholders than might be immediately obvious. To some extent, technical test analysts can bridge the gap between technology and the problem that technology solves during a review (and during dynamic testing).
Another important question for the technical test analyst is, Is there enough information in this work product to allow me to design, implement, and execute a test and, of course, to evaluate whether that test has passed or failed? Testability is a critical concern, but one that only testers tend to have at the top of their priority list. A testable requirement is one where you know what preconditions, steps, inputs, and data are necessary to evaluate the requirement, and the effort associated with doing so is not excessive.2 A testable requirement is also one where you could recognize the expected results and post-conditions associated with the test. Any requirement that doesn’t meet these two criteria should be flagged as a potential problem during a review.
While training is important, training participants by itself, however, is not enough. People might know the right thing to do. However, as discussed in Chapter 1, section 5 of the Foundation syllabus, people tend to align their actions and priorities with stated objectives and, especially, financial incentives and disincentives. If, for example, authors are measured and rewarded based on reducing the number of defects found in their work during reviews, static analysis, and dynamic testing, their incentive in a review (and during dynamic testing) will be to deny that any defect exists.
Rex has seen this kind of behavior firsthand, including aggressive and domineering verbalizations and physical postures toward testers. Not only must a rule be in place that reviews are positive experiences for authors—which is essential—there should be a similar rule that no attempts be made to invalidate a reviewer’s feedback. However, such rules are for naught if management punishes the discovery of defects in an author’s work or rewards the discovery of defects by reviewers.
Woody Allen, the New York film director, is reported to have once said, “Eighty percent of success is showing up.” That might be true in the film business, but just showing up would not make someone a useful review participant. Reviews require adequate preparation. If you spend no time preparing for a review, expect to add little value during the review meeting.
In fact, you can easily remove value by asking dumb questions that you could have answered on your own had you read the document thoroughly before showing up. You might think that’s a harsh statement, especially in light of the management platitude that “there are no dumb questions.” Well, sorry, there are plenty of dumb questions. Any question that someone asks in a meeting because of their own failure to prepare, resulting in a whole roomful of people having to watch someone else spend their time educating the ill-prepared attendee on something they should have known when they came in the room, qualifies as a dumb question. In fact, to us, showing up for a review meeting unprepared qualifies as rude behavior, disrespectful of the time of the others in the room.
What constitutes adequate preparation? Of course, you have to read the work product being reviewed completely and carefully and reflect on your understanding of it. Plan on this process taking longer than reading, say, a blog post or a newspaper article. You’ll need to take notes as you go about questions you have, problems you have found, and potential problems you need to check further prior to the review. You’ll need to check internal and external cross-references, because contradictions are frequent.
Even more challenging, you have to ask yourself, “What’s not here that should be here?” Omissions can be one of the hardest types of problems to find. By cross-referencing design elements, architecture, and code against higher-level specifications, you can find some of these problems. However, what does the user or customer want or need? Is it here? Will it work the way the user or customer expects?
Rex has seen reviews performed with inadequate or completely absent preparation time. People show up, listen politely, and make perfunctory comments. They point out minor spelling, grammar, and style problems and then agree to the result. That’s not a review; that’s an event that builds false confidence while making everyone happy about the harmonious process. The downstream results—finding many bugs that could have been detected and removed in a review—put the lie to this kind of success.
Why don’t people prepare adequately? In part, the answer is that they do not know how to prepare. Training can address this issue. However, a major problem is the lack of adequate time. Generally, someone should have a chance to spend at least one hour (to get into the flow) and at most two hours (to avoid attention span limits) in review preparation. This is a significant block of time. If someone is attending two or three reviews in a week, the preparation time according to this rule, plus the review time (which is similarly set at 1 to 2 hours), they could spend 12 hours (close to a third of the week) to carry out the reviews properly. A cursory job will take a lot less time, and perhaps people will feel good about it, but the value will not be obtained.
Because reviews are so effective when done properly, organizations should review every important document. That includes test documents. Test plans, test cases, quality risk analyses, bug reports, test status reports, you name it. Our rule of thumb is, anything that matters is not done until it’s been looked at by at least two pairs of eyes. You don’t have to review documents that don’t matter, but here’s a question for you: Why would you be writing a document that didn’t matter?
So, what can happen after a review? There are three possible outcomes. The ideal case is that the document is okay as is or with minor changes. Another possibility is that the document requires some non-trivial changes but not a re-review. The most costly outcome—in terms of both effort and schedule time—is that the document requires extensive changes and a re-review. Now, when that happens, keep in mind that, while this is a costly outcome, it’s less costly than simply ignoring the serious problems and then dealing with them during component, integration, system, or—worse yet—acceptance testing.
In an informal review, there are no defined rules, no defined roles, and no defined responsibilities, so you can approach these however you please. Of course, keep in mind that Capers Jones has reported that informal reviews typically find only around 20 percent of defects, while very formal reviews like inspections can find up to 85 percent of defects.3 If something is important, you probably want to have a formal review—unless you think that you and your team are so smart that no one is going to make any mistakes.
Like any test activity, reviews can have various objectives. One common objective is finding defects. Others, typical of most testing, are building confidence that we can proceed with the item under review, reducing risks associated with the item under review, and generating information for the team and for management. Unique to reviews is the addition of another common objective, that of ensuring uniform understanding of the document—and its implications for the project—and building consensus around the statements in the document. Reviews of test plans, test processes, and tests themselves can also reveal gaps in coverage.
Reviews usually precede dynamic tests. Reviews should complement dynamic tests. Because the cost of a defect increases the longer that defect remains in the system, reviews should happen as soon as possible. However, because not all defects are easy to find in reviews, dynamic tests should still occur.
5.2 Using Checklists in Reviews
Learning objectives
TTA 5.2.1 (K4) Analyze an architectural design and identify problems according to a checklist provided in the syllabus TTA 5.2.2 (K4) Analyze a section of code or pseudo-code and identify problems according to a checklist provided in the syllabus
We mentioned the role of technical test analysts in defining, applying, and maintaining checklists in the preceding section. Why are checklists important? For one thing, checklists serve to ensure that the same level and type of scrutiny is brought to each author’s work. There can be a tendency of review participants to defer to a senior employee, and thus that person’s work, when in fact everyone is fallible and we all make mistakes. Conversely, a less-senior or more-insecure employee might feel threatened by the review. Regardless of the individual author and their skills, there is nothing personal about locating potential problems and improvements for their work from a checklist. The checklists serve as a valuable leveling and depersonalizing tool, both factually and psychologically.
More important, though, checklists serve as a repository of best practices—and worst practices—that can help the participants of a review remember important points during the review. The checklist frees the participants from worrying whether they forgot some critical issue or mistake that was made in the past. Instead, the checklist gives general patterns and anti-patterns to the participants, allowing them to ask instead, “How could this particular item on the checklist be an important consideration for this work product that we’re reviewing?”
This idea of including not only best practices but also worst practices is important.4 Let’s illustrate with an example. In Chapter 4, we introduced the Common Vulnerabilities and Exposures site from MITRE, the Federally funded non-profit, reproduced again as Figure 5–2 below. If you are involved in reviewing code where security is important—and security almost always is important now—the items here can be incorporated into your code checklist and/or your static code analysis tools to ensure that dangerous coding mistakes are not happening.
Figure 5–2 List of top 25 security vulnerabilities from CVE website
What should go into a checklist? We’ll examine some specific items from the syllabus and from industry examples in subsections below, but we can make some general statements first. If there are common templates, style guides, variable naming rules, or word-usage standards in the company for certain types of work products, the checklists should reflect those guidelines. (Of course, the verification of some of these guidelines can be implemented in static analysis tools. In that case, the checklist or the review entry criteria can simply mention that successful completion of the static analysis is a pre-requisite to the review.)
Often it is the case that, for particular organizations or for particular systems, there are specific quality characteristics such as security and performance that are important and should be verified. So, checklists can include specific sections for each such characteristic, along with good and bad things to watch for. If these sections are applicable only to certain projects or products, the checklist should give guidance on when the items in a given section are important to consider.
While there are many useful examples of checklists for various kinds of work products, some of which we’ll present below, you should plan to maintain and evolve your organization’s checklists over time. When building on known best practices and worst practices in checklists you’ve adopted, keep in mind the following factors:
Product: Different products have different needs. As an extreme example of these contrasts, a smartphone application that allows users to play an entertaining and engaging game is a very different proposition than a mainframe application that handles thousands or even millions of mission-critical transactions per week.
People: Your team will have different strengths and weaknesses, which are often amplified by the nature of the product. If your team tends to make the same types of mistakes over and over, these should be part of your checklist, regardless of whether these mistakes are common in software engineering in general. In addition, the software development life cycle matters. To use an example provided by one reviewer, Bernard Homés, in an Agile model, a review should need to address not only new and changed functions in an iteration but also any existing functions from previous iterations that could be impacted.
Tools: If you are able to use static analysis tools to detect certain kinds of defects prior to the review, then the checklist can simply note whether the tools have been applied.
Priorities: What matters most to technical stakeholders, business stakeholders, users, customers, and testers should be considered as the checklist evolves.
History: You should look for anti-patterns in your defects. These should be part of your checklist. You should also look for successful examples—patterns—in your past releases. What does your organization do well? What does your organization do poorly?
Quality characteristics: You should look for ways in which your organization has successfully delivered various technical and domain quality characteristics. Can you see common elements in those successes? How about in the failures?
Testability: You should consider factors that make it easier, harder, or plain impossible to evaluate whether a requirement, user story, use case, design element, architectural attribute, or other attribute is or is not satisfied. While testability as a quality characteristic is not often the highest priority, it should be considered as part of any checklist. If you can’t determine whether something works or not, that’s going to be a problem during dynamic testing. Not only will there be a potentially large number of tests for a requirement with testability issues, it will also be hard to distinguish false positives (and false negatives) from actual defects.
While checklists are very useful, there are risks associated with them. One risk is whether it will focus attention on verification (are we building the product right?) rather than validation (are we building the right product?). Checklists will tend to bring your thinking into a verification mindset. Remember to think outside of the checklist to look at whether the product will actually solve the end users’ problems.
With these overall points in mind, let’s look at some specific examples of checklists. These examples should give you an idea of items to put in your own organization’s checklists.
5.2.1 Some General Checklist Items for Design and Architecture Reviews
Software architecture and software design are two terms that can cover a lot of territory. Let’s look at a few definitions of these terms before we start to discuss checklist items.
First, let’s take the syllabus definition of software architecture, which says it “consists of the fundamental organization of a system, embodied in its components, their relationships to each other and the environment, and the principles governing its design and evolution.”5 This definition can easily call to mind network diagrams of systems or the applications running on these systems, and how they communicate with each other. To be complete, though, you’d also need to see the principles behind the decisions related to the architecture. The architectural principles would (one would hope) be present in explanatory text surrounding the diagrams.
Next, a more expansive, perhaps more pragmatic, and definitely more assertive view of software architecture comes from the Software Engineering Institute:
The software architecture of a program or computing system is a depiction of the system that aids in the understanding of how the system will behave. Software architecture serves as the blueprint for both the system and the project developing it, defining the work assignments that must be carried out by design and implementation teams. The architecture is the primary carrier of system qualities such as performance, modifiability, and security, none of which can be achieved without a unifying architectural vision. Architecture is an artifact for early analysis to make sure that a design approach will yield an acceptable system. By building effective architecture, you can identify design risks and mitigate them early in the development process.
Notice the assertions of the importance of early architecture and the evaluation of architecture documents.6
So, what’s the difference between software architecture and software design? Well, Rex has heard these terms used interchangeably by various technical and business participants and stakeholders on projects, and a quick search on the Internet will reveal all sorts of opinions. To the extent that a difference is intended, it seems to be the level of focus. In that case, software architecture can refer to higher levels of component relationships, where the components can be databases, application servers, and other such coarse-grained elements. Software design can refer to components at a lower level, such as individual units or groups of units within a single application.
In the Advanced Technical Test Analyst syllabus, a list of general items to consider for an architecture review checklist is provided. This list is excerpted from an online resource, and in fact contains only a portion of the complete set of architectural attributes that the online resource says should be checked. The complete list includes the following attributes:
Performance: Elements of the architecture that affect response time and throughput.
Reliability: Elements of the architecture that ensure that the system operates properly over an extended period of time, even when confronted with invalid inputs, unexpected events, and so forth.
Availability: Related to reliability, these are elements of the architecture that ensure that the system stays up and running, which can include failover and redundancy.
Efficiency: Related to performance, reliability, and availability, and sometimes in tension with those priorities, these are elements of the architecture that enable it to use system resources in an optimal fashion.
Scalability: Related to performance, reliability, and efficiency, the elements of the architecture that make it possible to expand the number of users and to expand the hardware resources available to the system.
Security: Elements of the architecture that allow the system to reject unauthorized attempts to access data or functionality and to avoid denial of service.
Modifiability: Elements of the architecture that make it possible to update a system quickly, easily, and safely.
Extensibility: Related to modifiability, the elements of the architecture that make it easy to add new features or components or remove no longer needed features or components.
Reusability: Related to modifiability and extensibility, the elements of the architecture that make the system’s elements reusable for another system, which might or might not be an important consideration for any given system.
Maintainability: Related to modifiability, the elements of the architecture that enable quick, safe, and easy repair of defects discovered in production.
Portability: Elements of the architecture that help the system run in various environments.
Functionality: The elements of the architecture that allow the system to solve the business problems it is designed to solve in an accurate and suitable fashion.
Conceptual Integrity: The overall feel of the architecture, in terms of consistency across all the elements and attributes.
Interoperability: The elements of the architecture that relate to the system’s ability to communicate internally and externally, which often will have intersections with other attributes of the system architecture, such as security and performance.
Usability: The elements of the architecture that make the system easy to learn, operate, and understand and that make it attractive to the user, which in some cases must be balanced against other architectural priorities, such as security and performance.
Testability: The elements of the architecture that make the system easier to test at all test levels.
Ease of administration: The elements of the architecture that make the system easier for operators to manage, including backup and restore, data migration, account management, and so forth.
Debug-ability and monitoring: The elements of the architecture that make it easier to find defects and to monitor for operational anomalies, during both development and operation.
Development productivity: the elements of the architecture that make the developers more productive.
Specific questions and considerations are associated with each of these attributes, and the online resource helpfully provides metrics for each.7
5.2.2 Deutsch’s Design Review Checklist
Let’s look at an example of a checklist we can use to review distributed applications. The checklist comes from some work done by L. Peter Deutsch and others at Sun Microsystems in the 1990s. You might remember Sun’s early slogan: “The network is the computer.” Sun was in the forefront of distributed application design and development. Deutsch and his colleagues recognized that people designing and developing distributed applications kept making the same mistakes over and over again. They often made these mistakes because of eight typical, incorrect assumptions (which he called fallacies).
We can create a checklist based on Deutsch’s eight distributed application fallacies. The idea of a checklist is to force you to think. When you’re performing a review, we want you to think. That makes a checklist a good tool to use. Failing to consider these fallacies when designing a distributed application is guaranteed to cause issues once the application is delivered.
1. The network is reliable. It will never go down. Murphy once said what can go wrong, will go wrong. Heinlein’s corollary to that is, “Murphy was an optimist.” The network is made up of hardware and software. We already know that software can fail (we are, after all, professional pessimists). Anyone who has ever owned hardware knows that it can fail. Power can get interrupted, cables can get disconnected. People can do stupid things. In the long run, you can be assured that the network will occasionally go down; the question is what will happen to your application when it does.
2. Latency is zero. How long does it take for your data to go from here to there? It seems like it is fast—speed of light, right? If the network is local, it might be close to zero. What happens if it is a wide area network? The user is two continents away. Well, it is still pretty fast. Is it fast enough? What would need to be so fast anyway? In today’s world, you might be doing some distributed processing. Running web services from anywhere. Remote procedure calls from anywhere. When they are local, they are almost instantaneous; when they are remote, they are not. How is a delay in getting an answer to a called function going to affect the computation? What happens when that answer is needed in real time? How about if there are multiple processors waiting for the answer? There are suddenly failure possibilities due to timing. Are you holding locks while you wait? Hmmmm.
3. Bandwidth is infinite. Each year it is getting better—in most countries if not the United States. The United States is actually falling behind, although there are plans to bring us into the 21st century. As we travel around the United States teaching classes, it blows us away how many people are still on dial-up connections. So, when our fancy distributed application is downloading 5 megabytes of company logos to our customer’s computer through that 40 Kbps dial-up, they won’t mind a bit.
4. The network is secure. If you truly believe that, you might want to go back and review the technical security section in Chapter 4. As Jamie was writing this section, he looked up and saw that his virus checker icon in the toolbar had become disabled. Several times an hour that occurs when it is accessing the update center, so he did not think about it. Not until, that is, he looked at it minutes later and saw that it had not yet come back. Checking the other three computers in his office, Jamie saw that each was showing disabled icons. He immediately killed the network, shut down all four computers, and spent the next three hours going through each one trying to find the issue. When he couldn’t find any problems, he shut down for the night. It turned out to be a false alarm, but the fact is that, if you have been in computers for more than a couple of hours, you have heard of hackers, crackers, thrill seekers, and assorted mopes who break stuff just for kicks. The network is never secure.
5. Topology doesn’t change. At least, it doesn’t until you put the application into production. It doesn’t matter if your distributed application is in-house or worldwide; the only constant in networks is change. It may not change today, or tomorrow, but it will change. How long is your application going to be in use? Even the best prognosticators in the business have no idea what technical advances are going to come out next week or next year. The network must be abstracted away without depending on any given resource being constant.
6. There is one administrator. Sorry, there will be a bunch. Even if your application is in-house, people move on. What does it matter? Expertise, training, problem solving: all of these and more are going to be handled by the administrator of the system. Tools will be needed to solve issues with deployment. Where will they get the tools, the training, and the expertise? How user friendly will the system actually be? When you start looking at the details of deployment, you will have to look at the lowest common denominator and figure out how to deal with the administrators.
7. Transport cost is zero. Here you need to look at both time and money. To get the data from here to there takes time. This has to do with the idea of latency we discussed earlier. It takes time to go through the stacks of software, firmware, and hardware and then through the copper or fiber and finally through the hardware, firmware, and software at the other end. This takes time and processing cycles and those are not free. In addition, you are putting more load on already overloaded systems; there are times when new software and hardware are going to be needed. The less efficient your system, the more this will be true.
8. And finally, the network is homogenous. Writing a Windows application? What happens if your client uses Linux? Your application works in UNIX, but your client has a Series 5 app server? Proprietary protocols and services will get you every time. Interoperability is going to be essential to any application that is ever going to be moved outside the lab. Did you design for that?
Any review of a network application should include discussion of all these points.
5.2.3 Some General Checklist Items for Code Reviews
Technical test analysts are likely to be invited to code reviews. So, in the syllabus, a list of general items to consider for a code review checklist is also provided.8 As with any externally obtained checklist, you should tailor this list to your specific needs, considering the programming language(s) used in your organization, the major risks for your product, and whatever priorities your organization might have. As mentioned previously, the inclusion of best practices and worst practices (or patterns and anti-patterns, if you prefer) will be helpful in detecting problems.
Static code analysis tools can actually address a number of the items mentioned in the syllabus code checklist. It’s Rex’s opinion that all such items should be handled by such a tool and that you remove any item handled by the tool from the checklist. The most tedious discussions Rex has ever heard in code reviews involved stylistic questions such as, “Which line does the closing brace of a code block belong on?” and “Why aren’t you using Hungarian naming convention for your variable names?” Rex would almost rather shove a sharp pencil in his eye than sit through another such pointless discussion. The beauty of making these decisions about style and other such automatable checkpoints once, then enforcing them via a tool, is that you can permanently outlaw such ridiculous arguments from code review meetings.
Let’s look at some of the items that you might include in your code review checklist, starting with the general items from the syllabus.
The checklist starts with the structure of the code. It should be evaluated against any architecture and design specifications to ensure consistency and completeness. Look for any test software or test-enabling software. (Early in his career, when working as a programmer, Rex made the mistake of leaving a bunch of debug print statements in a piece of code that ultimately went to the customer, which proved quite embarrassing.) You should consider whether the use of memory is inefficient, such as allocating a very large multidimensional array that will only be sparsely populated with actual values. Look at any chunks of code to see whether they might correspond to library functions or system calls that could be used. The checklist also mentions coding standards, style, formatting, uncalled functions, unreachable code, repeated code, the use of hard-coded constants, and excessive complexity in this category, but Rex feels that all of these issues are best delegated to a static code analysis tool, as mentioned previously.
Next, the checklist looks at documentation of the code, which refers to commenting on the code. If there are any applicable style guidelines—for example, for class declaration and function headers—then those can often be checked to some extent by static code analysis tools. However, the comments are ultimately there to help people, usually programmers, understand and maintain the code, so the participants of the review should evaluate the comments carefully. The comments must make sense, describe what the code does in terms that make it clear why the code does what it does, and include all information necessary for ongoing maintenance of the code. The comments also should not simply parrot what is in the code. They should talk about why rather than what (which can be determined by reading the code itself).
The next category on the checklist is variables. Ideally, the kind of dataflow analysis discussed in Chapter 3 will be performed prior to the code review, so that any problems with dataflows are identified and removed first. Static code analysis tools should be able to detect unused variables, type mismatches of various sorts, and improperly named variables. So, checking whether the variable names are meaningful and add to the readability of the code is probably all the review need do, given the use of these other techniques.
It’s in computational and programming logic code that reviews really are without parallel, irreplaceable by tools, since these areas are where the programmer tells the computer what they want it to do. They are also the areas where some very serious bugs live. In terms of computations, it’s important to always remember that computers operate on values that are, in significant ways, different than the real-world or mathematical contracts they represent. Rounding errors will occur, and will accumulate and often be amplified, if care is not taken. Integers in a computer behave much like integers in the real world and in math, but there are those pesky upper and lower boundaries imposed by storage sizes. Floating-point numbers are even more constrained by the way in which computers represent them, which can lead to problems with equality comparisons. Division by zero is a risk. Overflow, underflow, and loss of information can happen when mathematical operations occur on numbers with very different sizes, such as dividing a very large floating-point number by a floating-point number very close to zero.
In terms of the programming logic, anytime the program makes a decision, we can have problems in the decision itself (as discussed in Chapter 2), or in the way in which the decision is coded, or in the failure to code some important alternative decision. Any branching, looping, or other logic construct must be checked to make sure nothing is missing, everything is correct, and, if any nesting occurs, the nesting is done properly. To aid efficiency, programming should be done to take advantage of short-circuiting and, in the case of if-elseif chains, the order of the checking, so that the cases most likely to occur are at the top of the chain and in the right place in any complex conditions to minimize condition evaluation. Check to make sure nothing is missing in any switch-case construct or if-elseif chain, including the final else or default.
Loops, being places where blocks of code are repeated dozens, maybe hundreds, maybe thousands of times, are very sensitive areas of a program and require the highest degree of scrutiny. Since an infinite loop can result in a hung application, check that each loop has clear conditions for termination and that those conditions will always be met at some point. If pointers, arrays, lists, stacks, or other such data structures will be accessed within the loop, make sure that they are properly initialized before the loop starts. Since each statement inside the loop consumes CPU cycles, check to see if anything inside the loop can happen outside the loop. Finally, if the loop uses an index or counter variable, such as a for loop in C and C++, that variable should only be used in the body of the loop, not set, and should not be used after the loop is completed.
Finally, there are certain programming practices, referred to as defensive programming, that help programs avoid abnormal termination, even when something goes awry within the program. Anytime a data structure such as an array, list, record, structure, or file is accessed, the program should check that it is not trying to access beyond the end of the list (e.g., the infamous buffer overflow bug). Any variable that will be used for an output, such as a return value from a function, should be initialized upon declaration, even if it seems impossible that the variable won’t be set later. When data is used or set, check that the program is acting on the right element; for example, when accessing a two-dimensional array, it’s easy to get the indices reversed. If heap memory is used in a language such as C or C++ where the programmer must manage the heap, check to make sure all heap variables are eventually released. (Of course, dynamic analysis tools are also useful in checking for such problems, and static code analysis tools will also watch for potential memory leaks.)
Finally, a lot can go wrong when interacting with the great wide wild world. Data coming into the program from an outside source, such as the user interface, an API, or a file produced by another program, should be validated, including making sure nothing is missing and nothing that shouldn’t be there is present. The program shouldn’t assume that external devices will respond or that files exist; time-outs, error traps, and checking files for existence before using them is smart. And, when the program is exiting—under whatever conditions, normal or abnormal—it should return the files, devices, and any other resources used to the correct state.
5.2.4 Marick’s Code Review Checklist
So, let’s go through Brian Marick’s code review checklist, which he calls a “question catalog.” This catalog has several categories of questions that developers should ask themselves when going through their code. These questions are useful for many procedural and object-oriented programming languages, though in some cases certain questions might not apply.9
For variable declarations, Marick says we should ask the following questions:
Are the literal values correct? How do we know?
Has every single variable been set to a known value before first use? When the code changes, it is easy to miss changing these.
Have we picked the right data type for the need? Can the value ever go negative?
For each data item and for operations on data items, Marick says we should ask the following questions:
Are all strings NULL terminated? If I have shortened or lengthened a string, or processed it in any way, did the final byte get changed?
Did we check every assignment to a buffer for length?
When using bitfields, are our manipulations (shifts, rotates, etc.) going to be portable to other architectures and endian schemes?
Does every sizeof() function call actually go to the object we meant it to?
For every allocation, deallocation, and reallocation of memory, Marick says we should ask the following questions:
Is the amount of memory sufficient to the purpose without being wasteful?
How will the memory be initialized?
Are all fields being initialized correctly if it is a complex data structure?
Is the memory freed correctly after use?
Do we ever have side effects from static storage in functions or methods?
After reallocating memory, do we still have any pointers to the old memory location?
Is there any chance that the memory might be freed multiple times?
After deallocation, are there still pointers to the memory?
Are we mistakenly freeing data we don't mean to?
Is it possible that the pointer we are using to free the memory is already NULL?
For all operations on files, Marick says we should ask the following questions:
Do we have a way of ensuring that each temp file we create is unique?
Is it possible to reuse a file pointer while it is pointing to an open file?
Do we recover each file handle when we are done with it?
Do we close each file explicitly when we are done with it?
For every computation, Marick says we should ask the following questions:
Are parentheses correct? Do they mean what we want them to mean?
When using synchronization, are we updating variables in the critical sections together?
Do we allow division by zero to occur?
Are floating-point numbers compared for exact equality?
For every operation that involves a pointer, Marick says we should ask the following questions:
Is there any place in the code where we might try to dereference a NULL pointer?
When dealing with objects, do we want to copy pointers (shallow copy) or content (deep copy?)
For all assignments, Marick says we should ask the following question:
Are we assigning dissimilar data types where we can lose precision?
For every function call, Marick says we should ask the following questions:
Was the correct function with the correct arguments called?
Are the preconditions of the function actually met?
Finally, Marick provides a couple of miscellaneous questions:
Have we removed all of the debug code and bogus error messages?
Does the program have a specific return value when exiting?
As you can see, this is a very detailed code review checklist. However, customization based on your own experience, and your organization’s needs, is encouraged.
5.2.5 The Open Laszlo Code Review Checklist
In the previous section, we discussed Marick’s questions that should be asked about the code itself. In this section, we will discuss questions that should be asked about the changes to the system. These are essentially meta-questions about the changes that occurred during maintenance. These come from the OpenLaszlo website.10
For all changes in code, here are the main questions we should ask:
Do we understand all of the code changes that were made and the reasons for them?
Are there test cases for all changes?
Have they been run?
Were the changes formally documented as per our guidelines?
Were any unauthorized changes slipped in?
In terms of coding standards, here are some additional questions to ask (assuming you are not enforcing coding standards via static analysis):
Do all of the code changes meet our standards and guidelines? If not, why not?
Are all data values to be passed parameterized correctly?
In terms of design changes, here are the questions to ask:
Do you understand the design changes and the reasons they were made?
Does the actual implementation match the designs?
Here are the maintainability questions to ask:
Are there enough comments? Are they correct and sufficient?
Are all variables documented with enough information to understand why they were chosen?
Finally, here are the documentation questions included in the Open Laszlo checklist:
Are all command-line arguments documented?
Are all environmental variables needed to run the system defined and documented?
Has all user-facing functionality been documented in the user manual and help file?
Does the implementation match the documentation?
We would also add one additional question that should be checked by the testers for every release: Do the examples in the documentation actually work? We have both been burned too often by inconsistencies—in some cases quite serious—between examples and the way the system actually works.
5.3 Deutsch Checklist Review Exercise
As you can see in the diagram at the beginning of the HELLOCARMS system requirements document, the HELLOCARMS system is distributed. In fact, it’s highly distributed, as multiple network links must work for the application to function.
In this exercise, you apply Deutsch’s distributed application design review checklist to the HELLOCARMS system requirements document.
This exercise consists of three parts:
1. Prepare: Based on Deutsch’s checklist, review the HELLOCARMS system requirements document, identifying potential design issues.
2. Hold a review meeting: Assuming you are working through this class with others, work in a small group to perform a walk-through, creating a single list of problems.
3. Discuss: After the walk-through, discuss your findings with other groups and the instructor.
The solution to the first part is shown in the next section.
5.4 Deutsch Checklist Review Exercise Debrief
Senior RBCS Associate Jose Mata reviewed an early version of the HELLOCARMS system requirements document (which did not include the nonfunctional sections). Jamie Mitchell reviewed the latest version of the document. Both used Wiegers’ checklist to guide their reviews.
The network is reliable. It will not go down, or will do so only very infrequently.
Jose: HELLOCARMS design in Figure 1 does not include any redundancy as in failover servers, backup switches, or alternate networking paths. Contingencies for how the system will not lose information when communication is lost are not defined.
Jamie: This version of the requirements still does not address system-wide reliability. Both fault tolerance (020-020-010) and recoverability (020-030-010) are seen only from the Telephone Banker’s point of view.
Latency is zero. Information arrives at the receiver at the exact instant it left the sender.
Jose: Efficiency was not discussed in the draft of the requirements that was reviewed.
Jamie: Time behavior is fairly well defined for the Telephone Banker front-end side (040-010-010, -060, -070 and -080). Some system-wide latency has been addressed (040-010-040 and -050). Other systems are not so defined.
Bandwidth is infinite. You can send as much information as you want across the network.
Jose: Efficiency was not discussed in the draft of the requirements that was reviewed.
Jamie: While the amount of information to be transmitted has not been defined (nor should it be in the requirements document), resource utilization has been addressed for the database server (040-020-010), the web server (040-020-020), and the app server (040-020-030). While some definitions are not complete, we would expect to clarify them based on the static review.
The network is secure. No one can hack in, disrupt data flows, steal data, and so on.
Jose: Section 010-040-010 doesn’t include enough detail to lend confidence.
Jamie: In this latest version, the security section has not changed. Overall security has not been well defined or (seemingly) considered.
Topology doesn’t change. Every computer, once on the network, stays on the network.
Jose: In the earlier version of the requirements document, fault tolerance and recoverability, in section 020, were TBD. The part regarding application is covered in section 010-010-060.
Jamie: Fault tolerance and recoverability are defined only at the Telephone Banker front-end level. Reliability for the rest of the system is currently underdefined.
There is one administrator. All changes made to the network will be made by this one person. Problems can be escalated to this one person. This person is infallible and doesn’t make mistakes.
Jose: Specific types of users, and their permissions, are not defined.
Jamie: Some attempts at defining usability (learnability: 030-020-010 and -020) have been made, but only at the Telephone Banker front-end level. Actual administration of the system has yet to be addressed.
Transport cost is zero. So, you can send as much information as you want and no one is paying for it.
Jose: There are no size limitations in section 010-010-160, “Support brokers and other business partners by providing limited partner-specific screens, logos, interfaces, and branding.” Some graphics can be large, if left undefined.
Jamie: Throughput values have been defined (040-010-110 to 040-010-160). The actual issue of cost of that throughput has not been defined.
The network is homogeneous. It’s all the same hardware. It’s all the same operating system. It’s all the same security software. The configurations of the network infrastructure are all the same.
Jose: Supported computer systems, operating systems, browsers, protocols, etc., are not defined. Versions should be specified, though perhaps this detail should be in a design document rather than a requirements specification.
Jamie: The new version of the requirements document does not change this.
5.5 Code Review Exercise
In this exercise, you apply Marick’s and the Open Laszlo code review checklists to the following code.
1. Prepare: Review the code, using Marick’s questions, Laszlo’s checklist, and any other C knowledge you have. Consider maintainability issues as you review the code. Document the issues you find.
2. Hold a review meeting: If you are using this book to support a class, work in a small group to perform a walk-through, creating a single list of problems.
3. Discuss: After the walk-through, discuss your findings with other groups and the instructor.
The solution to the first part is shown in the next section.
Here is the code that we are reviewing. This code performs a task by getting values from the user, performing a calculation, and then printing out the result. On subsequent pages, we will have a debrief for this exercise.
1. getInputs(float *, float *, float*);
2. float doCalcs(float, float, float);
3. ShowIt(float);
4. main(){
5. float base, *power;
6. float Scaler;
7. getInputs(&base, power, &Scaler);
8. ShowIt(doCalculations(base, *power));
9. }
10. void getInputs(float *base, float power, float *S){
11. float base, power;
12. float i;
13. printf(" Input the radix for calculation => ");
14. scanf("%f", *base);
15. printf(" Input power => ");
16. scanf("%f", *power);
17. printf ("/nScale value => ")
18. scanf("i", i);
19. *Base = &base;
20. *P = &power; }
21. float doCalcs(float base, float power, float Scale){
22. float total;
23. if (Scale != 1) total == pow(base, power) * Scale;
24. else total == pow(base, power);
25. return;}
26. void ShowIt(float Val){
27. printf("The scaled power value is %f W. ", Val);
28. }
5.6 Code Review Exercise Debrief
This code is representative of code that Jamie frequently worked with when he was doing maintenance programming. Rex will let Jamie describe his findings here.
Let’s start with some general maintainability issues with this code:
No comments.
No function headers. I have a standard that says that every callable function gets a formal header, explaining what it does, the arguments it takes, the return value, and what the value means. I also include change flags and dates, with an explanation for each change.
No reasonable naming conventions are followed.
I would prefer Hungarian notation11 so we can discern the data type automatically.
No particular spacing standards used, so code is not as readable as it might be.
Based on Marick’s checklist and a general knowledge of C programming weaknesses and features, here are some specific issues with this code:
Line 0: Not shown: We need the includes for the library functions we are calling. We would need stdio.h (for printf () and scanf()) and math.h (for pow()). These problems would actually prevent the program from compiling, which should be a requirement before having a code review.
Line 1: Every function should have a return value; in this case, void.
Line 2: No issues.
Line 3: Once again, the function should have a return value.
Line 4: This might work in some compilers, but the main should return a value (int), and if it takes no explicit arguments, it should have void. This is a violation of Marick’s miscellaneous question, “Does the program have a specific exit value?”
Line 5: The variable power is defined as a pointer to float, but no storage is allocated for it. Near as I can tell, there is no reason to declare it as a pointer, and it should simply be a local float declared. Note that these variables are passed in to a function call before being initialized. This could be seen as a violation of Marick’s declaration question, “Are all variables always initialized?” Since no data has been allocated, this is a violation of Marick’s allocation question, “Is too little (or much) data being allocated?” And, just to make it interesting, assuming that the code was run this way, it would be possible to try to dereference the pointer “*power”, which breaks Marick’s pointer question, “Can a pointer ever be dereferenced when NULL?”
Line 6: Variable is passed in to a function call before being initialized. This is a violation of Marick’s declaration question, “Are all variables always initialized?”
Line 7: The function call arguments are technically correct since the variable power was defined as a pointer. However, the way it is written it will blow up since there is no storage allocated. This is a violation of Marick’s allocation question, “Is too little (or much) data being allocated?” Since I would change power to a float in line 5, this argument would have to be passed in as “&power” just like the other arguments.
Line 8: Same issue with power; it should be passed by value as just “power”. Also, the function doCalculations() does not exist. It should be doCalcs(). And, if they are meant to be the same function, the argument count is incorrect.
Line 9: No issue.
Line 10: “S” is not a good name for a variable.
Line 11: The local variables have exactly the same name as the formal parameters passed in. I would like to think that this naming would prevent the module from compiling; I fear it won’t. It certainly will be confusing. If you must name the local variables the same as the parameters (considering the way they are used, it makes a little sense), then change the capitalization to make them explicitly different. I would capitalize the local variables Base and Power.
Line 12: While this is legal, it is a bad naming technique. The variable “I”, when used, almost always stands for an integer; here it is a float. At the very least it is confusing. This should likely match the others and be renamed, “Scaler”.
Line 13: No issue, although the prompt message is weak.
Line 14: No issue.
Line 15: No issue.
Line 16: No issue.
Line 17: The line feed is backwards: should be and not /n.
Line 18: We should be loading the value of S with this scanf() function. There is no need for the local variable i.
Line 19: Let me say that I hate pointer notation with a passion. Here, we are assigning a pointer to the value pointed to by *Base. What we really want to do is assign the actual value; the statement should read “*base = Base” (assuming we made the change in Line 11 to its name).
Line 20: Same as line 19, and I still really hate pointer notation. Also, we are not returning any value to the third argument of the getInputs() function. There should be a statement that goes, “*Scaler = scaler” (assuming we change the name of the variable as suggested in Line 12). *P is never declared; it is also a really poor name for a variable. Finally, the closing curly brace should not be on this line but moved down to the following line. That is the same indentation convention that we use for the other curly braces.
Line 22: No issue.
Line 23: We are doing an explicit equivalence check on a float [if (Scale != 1)]. This is a violation of Marick’s computation question, “Are exact equality tests used on floating-point numbers?” On some architectures, I would worry about whether the float representation of 1 is actually going to be equal to 1. The problem is that I really don't know what this scalar is supposed to do. It looks like the way the code is written, “Scale” is only there to save a multiplication if it is equal to 1. I would want to know if the user can scale at 5.3 (or any other real number) or if they could only use integers. If they could only input integers, I would change the data type to int everywhere it is used. If there is a valid reason to input a real number (i.e., one with a decimal), then I would lose the if statement and simply do the multiplication each time. Comments would help me understand the logic being used. The wrong operator has been used; it should be an assignment statement (=) rather than a Boolean compare (==).
Line 24: Incorrect operator; need a single equal sign.
Line 25: The calculation is being lost because we are returning nothing. It should return the local variable value, “total”.
Line 26: No issue.
Line 27: No issue.
While this is not required in the exercise, here’s the way Jamie rewrote the code to address some of these issues.
1. #include <stdio.h> // Need for I/O functions
2. #include <math.h>// Need for pow() function
3.
4. // We need to start with valid function prototypes
5. void getInputs(float *, float *, float * );
6. float doCalcs(float, float, float);
7. void ShowIt(float);
8.
9. // This program will prompt the user for 3 inputs. It will
10. // use those to calculate (Base ^^ Power) * Scaler.
11. // It will then print out the calculated value
12. int main(void){
13. float base, power, scaler;
14. getInputs(&base, &power, &scaler);
15. ShowIt(doCalcs(base, power, scaler));
16. }
17.
18. // This function prompts the user for 3 inputs and returns them
19. void getInputs(float *Base, float *Power, float *Scaler){
20. float base, power, scaler;
21. printf(" Input the radix for calculation => ");
22. scanf("%f", &base);
23. printf(" Input power => ");
24. scanf("%f", &power);
25. printf ("/nScale value => ")
26. scanf("%f", &scaler);
27. *Base = base;
28. *Power = power;
29. *Scaler = scaler;
30. }
31.
32. // This function performs the calculation
33. float doCalcs(float base, float power, float Scale){
34. float total;
35. if (Scale != 1) {
36. total = pow(base, power) * Scale;
37. }
38. else {
39. total = pow(base, power);
40. }
41. return total;
42. }
43.
44. // This function prints out the returned value
45. void ShowIt(float Val){
46. printf("The scaled power value is %f W. ", Val);
47. }
Finally, Open Laszlo’s checklist is concerned with changes, but there are some good rules there for any code. Let’s go through this one explicitly.
Main questions
• Do you understand the code? No! No idea what this code is doing.
• Are there test cases for all changes? No! No test cases were defined at all.
• Were the changes formally documented as per guidelines? Unknown; this might be new code.
• Were any changes made without new feature or bug fix requests? Unknown.
-
• Do the code changes adhere to the standards and guidelines? Absolutely not. No comments. No function headers. Poor naming of variables.
• Are any literal constants used (rather than parameterization)? Yes. On line 23, the literal constant “1” is used.
Design
• Do you understand the design? Unknown. No design document available.
• Does the actual implementation match that design? Unknown.
Maintainability
• Are the comments necessary? Accurate? No comments.
• Are variables documented with units of measure, bounds, and legal values? No.
Documentation
• Are command-line arguments and environmental variables documented? No.
• Is all user-visible functionality documented in the user documentation? No documentation.
• Does the implementation match the documentation? No documentation.
5.7 Sample Exam Questions
To end each chapter, you can try one or more sample exam questions to reinforce your knowledge and understanding of the material and to prepare for the ISTQB Advanced Level Technical Test Analyst exam. The questions in this section illustrate what is called a scenario question.
Walk-Through Scenario
Assume you are building an online application that allows for the secure transfer of encrypted financial data between banks, stock and bond trading companies, insurance companies, and other similar companies. This system will use public key infrastructure, and users will post their public keys on the system. The users’ private keys will be used by a thin client-side applet to decrypt the information on their local systems.
1. During preparation for a design specification walk-through, you notice the following statement:
A 10 MBPS or better network connection using TCP/IP provides the interface between the database server and the application server.
Suppose that the system under test will need to transfer data blocks of up to 1 gigabyte in size in less than a minute.
Which of the following statements best describes the likely consequences of this situation?
A. The system will suffer from usability problems.
B. The system will suffer from performance problems.
C. The system will suffer from maintainability problems.
D. This situation does not indicate any likely problems.
2. During preparation for a code inspection, you notice the following header in a member function for the object “ubcd”:
/* PURPOSE: Provides unsigned binary coded decimal integers,
* via a class of unsigned integers of almost
* unlimited precision. It can store a little over
* 4 billion 8 bit bytes, with each byte representing
* an unsigned pair of decimal digits. One digit is
* in each nybble (half-byte).
*/
Which of the following statements best describes why a programmer for a financial application would need to use such a binary coded decimal representation for data?
A. This approach ensures fast performance of calculations.
B. This approach indicates a serious design defect.
C. This approach maximizes memory resource efficiency.
D. This approach preserves accuracy of calculations.
3. During preparation for a peer review of the requirements specification, you notice the following statement:
The system shall support transactions in all major currencies.
Which of the following statements is true?
A. The statement is ambiguous in terms of supported currencies.
B. The statement indicates potential performance problems.
C. The statement provides clear transaction limits.
D. The statement indicates potential usability problems.