Coding Chica

When you write code, be nice to your future self (or whomever comes behind you and has to read / troubleshoot it). With both documentation and the code itself, I like to work with the mindset that I am going to get paged out in the future for some problem at 2am, when I am sleepy and impatient to go back to bed.

Also, using a consistent style throughout the code base makes it easier on the reader, even if Java would interpret the commands correctly with/without the formatting changes. While which style to use is a matter of personal / coding team preference, consistently adhering to that established standard shouldn’t be optional.

Consider the following 3 code snippets:

if(score > 60){System.out.println("Passed the class");}

if(score > 60)
{
    System.out.println(
        "Passed the class");
}

if(score > 60) {
    System.out.println(
       "Passed the class");
}

The Java compiler can understand the three versions of this code as equivalents. However, as people reading and possibly extending the code, one version may be easier for us to read and understand than the others. It isn’t that we cannot understand the other two, it is just that it takes our brains a little longer to figure out what it is trying to do. Now, imagine that snippet in a really long file containing hundreds or even thousands of lines of code. Making my brain work harder for each and every line just seems unnecessarily painful.

Now, let’s imagine that 3 different developers work on the same file and each has their own personal preferences. Then, we could end up with a strange mix of style flavors in the same class or method:

if (score > 60) {
    System.out.println("Passed the class");
} 
else if (score > 90) 
{
    System.out.println("Aced the class");
} else { System.out.println("Failed the class"); }

Again, Java may understand the statements correctly, but the next poor programmer trying to read this code will have to switch their minds expectations around to 3 different styles for the additions of 3 different developers. Consistency is important. Even if the team decides on a style you wouldn’t choose on your own, having a consistent style will help you as well when you go to troubleshoot some issue, since the formatting will be the same throughout. Be nice to your brain. Take out as much complexity as you are able. Make the styles consistent.

Enforcing Style in Peer Reviews

Imagine working hard on a change and the related unit tests and then opening a peer review to get consensus from the team…only to have a bunch of comments about style issues that you need to go back and update. Even worse, what if two members of the team disagree on what style should be used. Then, your change can’t be merged until the two peer reviewers agree…even if the code itself is correct.

Not to mention, all of the brain power spent looking at and commenting on the style of the change takes away from the time and energy the peer reviewers can spend looking at the most important aspect of the change – what it does and how it does it. If feedback is all about style, how confident are we that we got a good review of the change itself?

Maven Build Breaker – Checkstyle

The Maven Checkstyle Plugin allows the team to establish a common set of style rules and will fail the build if those expectations are not met. This is my preferred approach.

• As a peer reviewer: I may hold off doing my review until the build succeeds. That way, I can focus on what the code is doing and how the code is doing it.
• As a commit author: As long as my build succeeds locally, I may be ready to open my peer review. I have clear expectations for what style is expected of me and can easily tell when those expectations have been met.

For now, let’s use the default configuration, so we at least have consistency in the code we add. We can look at customizing the checks enforced in a subsequent post.

Update Pom.xml

The basic change we need to make to the pom.xml is to add the new plugin’s configuration to the build’s plugins section:

     <plugin>
       <groupId>org.apache.maven.plugins</groupId>
       <artifactId>maven-checkstyle-plugin</artifactId>
       <version>3.3.0</version>
       <configuration>
         <!-- use default configuration for now
            TODO - Create our own configuration file and reference here
            <configLocation>checkstyle.xml</configLocation> 
         -->
         <encoding>UTF-8</encoding>
         <consoleOutput>true</consoleOutput>
         <failsOnError>true</failsOnError>
         <linkXRef>false</linkXRef>
       </configuration>
       <executions>
         <execution>
           <id>validate</id>
           <phase>validate</phase>
           <goals>
             <goal>check</goal>
           </goals>
         </execution>
       </executions>
     </plugin>

The snippet above is a slightly modified version from maven-checkstyle-plugin’s usage page. I commented out the configLocation and added a TODO, so we can circle back to this in the future.

Run Maven Build – Failure

Even for experienced developers, I would be highly surprised if we add a style checker to the build and it succeeds on the first run. This is again, something that is easiest to add near the beginning of a project’s development…there are fewer issues that you must fix in the existing code before being able to commit.

The good news, is that we want to see the failure anyway. That way we can confirm that the new plugin is configured correctly and will actually stop the build from succeeding if we encounter issues.

In the java101 project, when I run the mvn clean install command to perform the build, I see output like:

[INFO] --- checkstyle:3.3.0:check (validate) @ java101 ---
[INFO] Starting audit...
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:1: Missing package-info.java file. [JavadocPackage]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:14:27: Parameter byteValue1 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:14:44: Parameter byteValue2 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:25:27: Parameter shortValue1 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:25:46: Parameter shortValue2 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:36:27: Parameter intValue1 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:36:42: Parameter intValue2 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:39: Line is longer than 80 characters (found 126). [LineLength]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:41: Line is longer than 80 characters (found 125). [LineLength]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:51:16: Parameter name should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:51:23: 'name' hides a field. [HiddenField]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:61: Line is longer than 80 characters (found 82). [LineLength]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:74:29: Parameter args should be final. [FinalParameters]
Audit done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
...
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:check (validate) on project java101: Failed during checkstyle execution: There are 13 errors reported by Checkstyle 9.3 with sun_checks.xml ruleset. -> [Help 1]

Let’s examine the parts of that output that are important. After the BUILD FAILURE message, we see how many failures there were and what rule set is being used:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:check (validate) on project java101: Failed during checkstyle execution: There are 13 errors reported by Checkstyle 9.3 with sun_checks.xml ruleset. -> [Help 1]

If we look higher in the output, it shows us the details of those 13 failures:

[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:1: Missing package-info.java file. [JavadocPackage]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:14:27: Parameter byteValue1 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:14:44: Parameter byteValue2 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:25:27: Parameter shortValue1 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:25:46: Parameter shortValue2 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:36:27: Parameter intValue1 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:36:42: Parameter intValue2 should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:39: Line is longer than 80 characters (found 126). [LineLength]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:41: Line is longer than 80 characters (found 125). [LineLength]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:51:16: Parameter name should be final. [FinalParameters]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:51:23: 'name' hides a field. [HiddenField]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:61: Line is longer than 80 characters (found 82). [LineLength]
[ERROR] ...\java101\src\main\java\codingchica\java101\App.java:74:29: Parameter args should be final. [FinalParameters]

The format taken by each of the 13 reports is:

<FilePath>:<LineNumber(s)>: <Message> [<CheckName>]

Next, we need to understand the issues reported and fix each one.

As shown on the maven-checkstyle-plugin’s introduction page, the checks that it performs are described within https://checkstyle.org. I like to use the Checkstyle.org’s Checks page as my jumping off point when troubleshooting issues like these. It has a list of the available checks.

Fixing JavadocPackage

The JavadocPackage check enforces that we have package (or folder) level documentation about the purpose of files in that folder.

Let’s add a package-info.java to the codingchica.java101 folder:

/**
 * Code demonstrating the basics of the Java language.
 */
package codingchica.java101;

Notice the empty line at the end of the file. This is another check that the maven-checkstyle-plugin enforces.

If we rerun the build after our update, we should no longer see this issue in the build output.

Fixing FinalParameters

When an input parameter is marked as final, the Java compiler enforces that no change in the value of that variable occurs within the method. For primitives, like we’re using now, it means that the raw value stored is unchanged. For objects, it is a little different – it only means that we haven’t changed what object we are pointing at…but doesn’t guarantee that no changes occurred during the method’s execution. More on that later.

In order to fix this style issue, let’s add the final keyword to each of the parameters it suggests. For example, in App.java, the method signature for the byte-adding method would change for both of the input parameters:

public static int add(final byte byteValue1, final byte byteValue2) {

Again, if we rerun the build, we should no longer see these items reported.

Fixing LineLength

In this case, the default sun checks enforces that each line of code is no longer than 80 characters. We need to break our commands / instructions onto multiple lines if they exceed this limit. Typically, the new line(s) being added to continue the command will also be further indented, to show visibly that they continue an existing Java command.

        if (sum < Integer.MIN_VALUE) {
            throw new IllegalArgumentException(String.format(
                    "int underflow with addends:  %s and %s", intValue1, 
                    intValue2));
        } else if (sum > Integer.MAX_VALUE) {
            throw new IllegalArgumentException(String.format(
                    "int overflow with addends:  %s and %s", intValue1, 
                    intValue2));
        }

However, when wrapping a comment, we do not need the indenting for the new line:

    /**
     * Retrieve the greeting that should be outputted when the application is
     * run.
     *
     * @return The greeting to output.
     */

If we run the build again, we should no longer see these issues reported.

Fixing HiddenField

This check is trying to prevent situations where a developer forgets to add a this. prefix before a field reference and accidentally gets a different variable (such as an input parameter or local variable within a method) instead. Therefore, it is asking that one of them be renamed to avoid the possible accidental name collision. In this case, let’s change the input parameter name to newName. In setters (or this constructor) adding the new prefix to the input parameter value still makes sense and allows for the unique naming:

    /**
     * Constructor for the App class.
     *
     * @param newName The name to use in the application's execution.
     */
    public App(final String newName) {
        this.name = newName;
    }

Build Success

After fixing these issues, we should now see a BUILD SUCCESS when running the build.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  7.096 s

You may notice that the build time jumped from around 2-3 seconds in the last post to around 7 seconds above. Each step / plugin you add to the build is going to make the build take longer. In this case, I think the extra few seconds in the build is an acceptable trade-off for both:

• Peer review manual review costs for both author and peer reviewer, and
• Troubleshooting time delays due to inconsistently formatted code

Commit

Now is a good time to commit.

Team Coordination

If working with a team, any branches or peer reviews they have in-flight may need to be updated to consume this update before they can be successfully merged back into the main code base/branch. Merge this change into the main code carefully and coordinate with the rest of the team, so this change does not cause issues for others.