SQA – Software Quality Assurance
Lecture 6
Source Code Review
Faculty of Information Technology
Hanoi University
Reviews & Inspections
• Code Review or Program Inspection is the
checking of source code to find bugs and improve
code quality.
– It is done as part of Reviews & Inspections.
• Other Review & Inspection activities:
– Design review
– Software product review
– Documentation review
– Process review
Overview & Objectives
• To discover & fix errors.
• To make source code easier to understand.
• To make software easier to maintain.
• Two levels:
– High level: code architecture
– Low level: specific code
Organization of software project
• Have a well-defined package/folder structure
in software project.
– Create a clear hierarchy of packages and resources
folders.
– Put relevant classes in the same package.
– Put relevant resources in the same folder.
– Separate resources from source codes, software
modules from components...
Managing relationships
among software components
• Coupling Between Object Classes
• Depth of Inheritance Tree
• Number of methods per class
• Method fan-in/fan-out
Classes
• Should be small
• Should have single responsibility
• Should have small number of instance
variables
Coupling Between Object Classes
• The measure of how many other classes rely on the
class and vice versa.
– Two classes are considered coupled when methods
declared in one class use methods or instance variables of
the other class.
• Large values imply:
– More complexity, Reduced maintainability, Reduced
reusability
• What to do?
– Split the class into smaller ones if possible
– Mark as ‘highly dependent’, put more maintenance efforts
Depth of Inheritance Tree
• The number of inheritance layers that make
up a given class hierarchy.
• Large values imply more design complexity.
– Many object classes may have to be understood to
understand the object classes at the leaves of the tree.
• What to do?
– Consider carefully before an attempt to increase DIT.
Number of methods per class
• An indicator of the amount of effort required to
implement and test a class.
– High values suggest that the class is too large, difficult to
understand and maintain.
• Weighted methods per class
– Complex methods are given more weight.
• What to do?
– Split methods into smaller methods.
– Move methods into suitable classes.
– Split class into smaller classes.
Fan-in/Fan-out
• Fan-in of method X: the number of methods that call
method X.
– A high value means that changes to X will have extensive
effects on other parts of the program.
• Fan-out of method X: the number of methods that
are called by method X.
– A high value suggests that the overall complexity of X is
high.
Clean Code
LOW-LEVEL CODE REVIEW
Why clean code matters?
• Much more time is spent reading code than
writing code.
– The ratio is said to be well over 10:1
• Clean code makes it easier to write more code.
• Bad code is a big cause of software failure.
– With bad code, there will be a point where we can no
longer maintain it.
The Boy Scout Rule
• Leave the campground cleaner than you found it.
(The Boy Scouts of America)
• Always leave the code you’re editing a little
better than you found it. (Robert C. Martin)
• It doesn’t have to be a big change!
– Change one variable name
– Break up one large function
– Remove a bit of duplication
– ...
What can a name tell you?
• A name can be used for:
– A variable, a function, a class, etc.
• It may be able to indicate:
– Why it exists?
– What it does?
– How it is used?
Meaningful Names
• Example of bad variable naming:
int d; // elapsed time in days
• Suggestions:
int elapsedTimeInDays;
int daysSinceCreation;
int daysSinceModification;
int fileAgeInDays;
• What have been impoved?
– Unit of measurement: day
– Related context: file age, creation date, modification date
Meaningful Names
public List<int[]> getThem() {
List<int[]> list1 = new ArrayList<int[]>();
for (int[] x : theList)
if (x[0] == 4)
list1.add(x);
return list1;
}
• Examples of bad names:
– list1, theList, getIt, getThem...
Meaningful Names
public List<int[]> getThem() {
List<int[]> list1 = new ArrayList<int[]>();
for (int[] x : theList)
if (x[0] == 4)
list1.add(x);
return list1;
}
• Examples of bad names & values:
– list1, theList, getIt, getThem...
– x[0], 4
• What are the missing pieces of information?
Meaningful Names
public List<int[]> getFlaggedCells() {
List<int[]> flaggedCells = new ArrayList<int[]>();
for (int[] cell : gameBoard)
if (cell[STATUS_VALUE] == FLAGGED)
flaggedCells.add(cell);
return flaggedCells;
}
• What have been done?
– List’s name indicates what it contains
– Named constants
Meaningful Names
public static void copyChars(char a1[], char a2[]) {
for (int i = 0; i < a1.length; i++) {
a2[i] = a1[i];
}
}
• What’s the difference between a1 and a2?
public static void copyChars(char src[], char dest[]) {
for (int i = 0; i < src.length; i++) {
dest[i] = src[i];
}
}
Meaningful Names
• Type encoding is not necessary
PhoneNumber phoneString;
• Above is the result of type change not followed
by name change.
Class Names
• Use specific names:
– Customer, SearchResult, WikiPage,
AddressParser...
• Avoid ambiguous names:
– Manager, Processor, Data, Info...
Functions (methods)
• General principles:
– Small!
– Do one thing
– Contain one level of abstraction
– Avoid side effects
– Use exceptions over error codes
Functions should be small
• Should not contain > 2 levels of nested structures
– i.e. conditionals, loops...
• Long blocks within conditionals or loops should
be turned into a function.
public static String renderPageWithSetupsAndTeardowns(
PageData pageData, boolean isSuite) throws Exception {
if (isTestPage(pageData))
includeSetupAndTeardownPages(pageData, isSuite);
return pageData.getHtml();
}
Example of a function containing high-level logics
One level of abstraction per function
• Avoid putting different levels of abstraction in the
same function (method).
// high level of abstraction
pageData.getHtml();
// intermediate level of abstraction
String pagePathName = PathParser.render(pagePath);
// low level of abstraction
buffer.append("\n");
Do one thing
• The steps of the function should contribute to a
single goal.
– i.e. statements are one level of abstraction below the
name of the function
WikiPage wikiPage = pageData.getWikiPage();
StringBuffer buffer = new StringBuffer();
if (pageData.hasAttribute("Test")) {
WikiPagePath pagePath = getFullPath();
String pagePathName = PathParser.render(pagePath); Can be put in
buffer.append("!include -setup ."); a separate
buffer.append(pagePathName); function.
buffer.append("\n");
}
Avoid side effects
public boolean checkPassword(String userName, String password) {
User user = UserGateway.findByName(userName);
if (user != User.NULL) {
String codedPhrase = user.getPhraseEncodedByPassword();
String phrase = cryptographer.decrypt(codedPhrase, password);
if ("Valid Password".equals(phrase)) {
Session.initialize();
return true;
}
}
return false;
}
• Rename function to describe side effect
– e.g. checkPasswordAndInitializeSession
• Or include side-effect warning comment
Reading Code from Top to Bottom
The Stepdown Rule
• Every function should be followed by those at the
next level of abstraction.
– Smaller functions should be easily found right below the
high-level one.
Good Comments: Clarification
assertTrue(a.compareTo(a) == 0); // a == a
assertTrue(a.compareTo(b) < 0); // a < b
assertTrue(ab.compareTo(ab) == 0); // ab == ab
// This is our best attempt to get a race condition
// by creating large number of threads.
for (int i = 0; i < 25000; i++) {
Thread thread = new Thread();
thread.start();
}
public int compareTo(Object o) {
if(o instanceof Student) {
Student s = (Student) o;
return s.getName().compareTo(o.getName());
}
return 1; // Students are greater than everything else.
}
Good Comments: Warning
// Don't run unless you have some time to kill.
public void _testWithReallyBigFile() {
writeLinesToFile(10000000);
}
// SimpleDateFormat is not thread safe,
// so we need to create each instance independently.
SimpleDateFormat df =
new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss z");
df.setTimeZone(TimeZone.getTimeZone("GMT"));
Bad Comments: Insignificant
/**
* Default constructor.
*/
protected AnnualDateRule() {
}
/**
* Returns the day of the month.
*
* @return the day of the month.
*/
public int getDayOfMonth() {
return dayOfMonth;
}
The importance of code formatting
• Keep each line not longer than screen width.
• Have blank lines to separate the package declaration,
imports, and each of the methods.
• Keep related lines of code close together.
public class FitNesseServer implements SocketServer {
private FitNesseContext context;
public FitNesseServer(FitNesseContext context) {
this.context = context; }
public void serve(Socket s) {
serve(s, 10000); }
public void serve(Socket s, long requestTimeout) {
try {
FitNesseExpediter sender = new FitNesseExpediter(s, context);
sender.setRequestParsingTimeLimit(requestTimeout);
sender.start(); }
catch(Exception e) {
e.printStackTrace(); }
} }
References
• Roger S. Pressman, Bruce R. Maxim, 2020.
Software Engineering - A practitioner’s
approach, 9th edition. McGraw Hill.
• Robert C. Martin, 2008. Clean Code - A
Handbook of Agile Software Craftsmanship.
Prentice Hall.
