Static analyzers are essentially tools that perform an automated code review. A static analyzer looks at code before it is executed and checks for certain quality standards, often a checking for the adherence to a specific coding standard, as well as checking for other common vulnerabilities that could result in bugs. Although similar standards can be enforced through manual code review, using a static analyzer is faster, more accurate, and frees up developer time that would be otherwise be spent performing nit-picky code reviews.

We will use two static analyzers (Checkstyle and SpotBugs) on BroadleafCommerce's Profile Core Service and discuss their similarities and differences.

Checkstyle has an option to use Google Checks or Sun Checks. The difference between the two is that the code is checked against two different standards, either the Google Java Style or Sun's Java Style. Not being entirely sure which one BroadleafCommerce might be following, I ran the checks using both. In 103 files, Google Checks found 5,008 issues and Sun Checks found 4,216 issues.

The Google Checks console display is slightly different from the Sun Checks display, visable in the images below:

Regardless of Google Checks or Sun Checks, the Checkstyle console provides a list of each error organized by file. Double-clicking on the error opens the file to the line of code that is causing the issue.

Before analyzing these in more detail, we will first look at another static analyzer.

SpotBugs only found 4 errors in 122 classes. All four errors looked very similar and were of medium confidence.

The SpotBugs console provides a file system where the vulnerabilities can be found, the specific line of code that the vulnerability is in, and a description of the vulnerability.

The file system lets us see the offending files:

Clicking on one of the files shows us the specific line of code causing the issue:

We also get a description of the error and additional information:

Clicking on the yellow bug on the left column of a file displays a description of the error and an explanation of how it might be an issue:

There was clearly a large difference between Checkstyle and SpotBugs, given that one found over a thousand times the number of issues than the other. The reason for this is that SpotBugs is only looking for specific patterns that often lead to bugs - there are 400 patterns that they look for, as specified in their documentation. These 400 patterns seem more focused on looking at specific potential logic flaws rather than on syntatic choices. For example, in the images above, SpotBugs finds an issue where setCreateDate ends up storing a mutable object. The issue here is that someone can accidentally (or maliciously) edit the date after it is stored. In the following code (based heavily on this helpful stack overflow response), we can edit the year of the date after setting it, and it would end up editing the date that we stored as well.

CustomerForgotPasswordSecurityToken customerForgotPasswordSecurityToken = new customerForgotPasswordSecurityTokenImpl();

Date dateToBeSet = new Date();
customerForgotPasswordSecurityToken.setCreateDate(dateToBeSet);

dateToBeSet.SetYear(...)

To fix this, we would need to edit the setter to deep-copy before setting the date. Please see the following code sample (again, heavily borrowed from the same helpful stack overflow response), for an example.

public void setCreateDate(Date createDate) {
    this.createDate = new Date(createDate.getTime());
}

This issue is similar to the other three found by SpotBugs in this module. The issues that SpotBugs finds, or at least the four that were found in this module, are very helpful, as they seem like they might actually cause unexpected behavior in the code.

Checkstyle, on the other hand, encompasses checks on adhering to a specific coding standard, such as Google Java Style or Sun's Java Style. These include syntactic or styling issues, such as having too many characters on a line or missing a Javadoc comment. Given the high number of errors, BroadleafCommerce does not appear to be following either of these standards. It is possible to configure Checkstyle to use a different coding standard for its checks, but I was unable to find and implement the standard that BroadleafCommerce is following, if any.

Due to BroadleafCommerce not following the two styles that Checkstyle checked against, it does not seem particularly useful in this example. However, given the correct standards to check against (or if used earlier in BroadleafCommerce's development), they would be very useful for improving the readbility and understandability of the code, which would also improve the maintainability of the code. From the work I already completed in BroadleafCommerce's Profile Core Service, I have personally experienced the frustrations of trying to read a line of code that extends much longer than the width of my laptop screen and trying to understand the purpose and functionality of functions with missing Javadocs.

SpotBugs and Checkstyle did not have any overlapping issues found. However, they both seem useful in different ways. SpotBugs seems good for finding logic patterns that could result in strange behavior, while Checkstyle seems good for ensuring that developers stick to a coding standard that will make it easier for other or future developers to work on the same code.