Turning functional code into great code: Part 1: Testing

I recently came into contact with an eager young developer with a year or two of work experience, who showed me his code and asked me how to improve it. Since I've been asked this by many people over the course of many years, I thought I might be useful for other young (or not-so-young!) developers who wish to improve their code also. I plan to break this into multiple parts, providing explanations and brief justifications for those who may be new to various best practices, even basic ones. Hope you stay tuned!

Caveat: I am not the world's foremost expert in, well, much of anything, and I have formed various opinions and biases over the years. So if you're a senior developer or architect, I'm sure you'll see something here that either I missed or you disagree with. Please feel free to let me know in the comments and I'll be happy to discuss it further. Also please note that I'll only be covering testability in this part, and will gradually introduce more sophisticated improvements in future installments.

Okay, without further ado, let's get started! The code in question builds an XSLT stylesheet for validation and processing of data contained in an XML file.

StylesheetCreationAction.java

package net.galluzzo.example.stylesheetbuilder.old;

import java.io.FileOutputStream;
import java.io.PrintStream;
import java.util.ArrayList;
import java.util.List;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.apache.struts.action.Action;
import org.apache.struts.action.ActionForm;
import org.apache.struts.action.ActionForward;
import org.apache.struts.action.ActionMapping;

public class StylesheetCreationAction extends Action
{
    String stylesheetDirectory = "C:\\apache-tomcat\\webapps\\stylesheet-builder\\xslt";
    
    public ActionForward execute( ActionMapping mapping, ActionForm form,
        HttpServletRequest request, HttpServletResponse response )
    {
        //...
        
        createStylesheet( parentNodeName, childNodeNames, ruleList,
            outputFilename );
        
        return new ActionForward( "success" );
    }

    public void createStylesheet( String parentNodeName, String childNodeNames,
        List ruleList, String outputFilename ) throws Exception
    {
        System.out.println( "Inside createStylesheet" );
        
        StylesheetHelper stylesheetHelper = new StylesheetHelper();

        String ruleContents = "";
        System.out.println( "Number of rules: " + ruleList.size() );

        for ( int z = 0; z < ruleList.size(); z++ )
        {
            ArrayList rule = (ArrayList) ruleList.get( z );

            String columnName = (String) rule.get( 1 );
            int ruleId = ( Integer.parseInt( rule.get( 2 ) + "" ) );
            String param1 = (String) ( rule.get( 3 ) );
            String param2 = (String) ( rule.get( 4 ) );
            String params = "";
            if ( ruleId == 1 || ruleId == 2 || ruleId == 3 || ruleId == 5 )
            {
                params = param1;
            }
            else
            {
                params = param1 + "," + param2;
            }
            System.out.println( "Rule table values: " + columnName + " "
                + ruleId + " " + params );

            switch ( ruleId )
            {
                case 1: // concat
                {
                    System.out.println( "Case 1" );
                    ruleContents += stylesheetHelper.concatRule( z + 1,
                        columnName, params );
                    break;
                }
                case 2: // greater than
                {
                    System.out.println( "Case 2" );
                    if ( params.indexOf( "-" ) < 0 )
                    {
                        int paramInt = Integer.parseInt( params );
                        ruleContents += stylesheetHelper.greaterThanRule(
                            z + 1, columnName, paramInt );
                    }
                    else
                    {
                        ruleContents += stylesheetHelper.greaterThanRule(
                            z + 1, columnName, params );
                    }
                    break;
                }
                // Several other cases here which we'll get to later
            }
        }

        // Get header and footer stylesheet content.
        String stylesheetHeader = stylesheetHelper.getStylesheetHeader(
            parentNodeName, childNodeNames );
        String stylesheetFooter = stylesheetHelper.getStylesheetFooter( parentNodeName );

        // Get the beginning and ending text for the <xslt:choose> node.
        String xsltChooseStart = stylesheetHelper.getXsltChooseStart();
        String xsltWhenEnd = stylesheetHelper.getXsltWhenEnd();
        String xsltOtherwiseStart = stylesheetHelper.getXsltOtherwiseStart();
        String xsltChooseEnd = stylesheetHelper.getXsltChooseEnd();

        // Get XML nodes.
        String parentNode = stylesheetHelper.getParentNode();
        String rejectedParentNode = stylesheetHelper.getRejectedParentNode();

        // Construct the entire stylesheet.
        String xsltText = stylesheetHeader + ruleContents + xsltChooseStart
            + parentNode + xsltWhenEnd + xsltOtherwiseStart
            + rejectedParentNode + xsltChooseEnd + stylesheetFooter;

        // Create the output file.
        FileOutputStream fo = new FileOutputStream( stylesheetDirectory
            + outputFilename );
        PrintStream ps = new PrintStream( fo );
        ps.println( xsltText );
    }
}

We will cover the StylesheetHelper class in a future installment.

Testability

The goal of this series of blog posts is to improve this code. But how do I do that and still ensure that the refactored code still does what the old code did? By writing a unit test, then making changes, and after each change, ensuring that the test still passes.

Unit tests are programs that test a "unit" of code, usually a class, by executing various functionality (methods) on it, and programmatically ensuring that the results are correct. I will use the JUnit 4 framework in this example, for the simple reasons that I know it fairly well, and it is widely used.

The problem is that I cannot actually run the above class on my machine. You see, I am writing this blog post on Linux, and the final code inside createStylesheet writes out a file into a specific Windows directory that is hard-coded into the class. So, for the very smallest change possible, for now, I will change this directory to one that works on my machine. (This is not a good solution, but we'll get to that later.)

Furthermore, this class is a Struts action, which means that it must extend from Struts' Action base class in order to do any work. I don't have a copy of Struts 1.x handy, and frankly I should not need it in order to build an XSLT stylesheet file from a list of passed-in rules. Happily, the createStylesheet method is the one I really care about, and it only uses Java APIs, not Struts or J2EE. The creation of a stylesheet is functionality that can be reused outside of a single web action, so it should go in a non-J2EE-related class. We'll call this class StylesheetCreator, thus:

public class StylesheetCreator
{
    String stylesheetDirectory = "/home/eric/workspace/stylesheet-builder/resources/output/";

    public void createStylesheet( String parentNodeName, String childNodeNames,
        List ruleList, String outputFilename ) throws Exception
    {
        System.out.println( "Inside createStylesheet" );
        // The rest of the original method goes here.
    }
}

With that simple change, we can now write a test case for this functionality, which looks like this:

package net.galluzzo.example.stylesheetbuilder.refactored.part1;

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import org.junit.Test;

/**
 * Test case for {@link StylesheetCreator}.
 * 
 * @author Eric Galluzzo
 */
public class StylesheetCreatorTest
{
    private static final String STYLESHEET = // ...long XML string here

    @Test
    public void testAllRules() throws Exception
    {
        StylesheetCreator stylesheetCreator = new StylesheetCreator();

        List<List<String>> rules = new ArrayList<List<String>>();
        rules.add( createRule( "name",       1,  ", Esq.", "" ) );
        rules.add( createRule( "age",        2,  "18",     "" ) );
        rules.add( createRule( "category",   2,  "-aaa",   "" ) );
        rules.add( createRule( "age",        3,  "100",    "" ) );
        rules.add( createRule( "category",   3,  "-zzz",   "" ) );
        rules.add( createRule( "department", 4,  "old",    "new" ) );
        rules.add( createRule( "category",   5,  "4",      "" ) );
        rules.add( createRule( "bonus",      16, "100",    "5000" ) );
        rules.add( createRule( "category",   16, "-aaa",   "-zzz" ) );

        stylesheetCreator.createStylesheet( "employee",
            "name,age,salary,category,department,bonus", rules, "test.xsl" );

        String stylesheetText = readFileContents(
            new File( stylesheetCreator.stylesheetDirectory, "test.xsl" ) );
        assertThat( stylesheetText, is( STYLESHEET ) );
    }

    private List<String> createRule( String columnName, int ruleId,
        String param1, String param2 )
    {
        List<String> rule = new ArrayList<String>();
        rule.add( "employee" );
        rule.add( columnName );
        rule.add( String.valueOf( ruleId ) );
        rule.add( param1 );
        rule.add( param2 );
        return rule;
    }

    private String readFileContents( File file ) throws IOException
    {
        StringBuilder sb = new StringBuilder();
        BufferedReader reader = new BufferedReader( new FileReader( file ) );
        try
        {
            String line;
            while ( ( line = reader.readLine() ) != null )
            {
                sb.append( line ).append( "\n" );
            }
        }
        finally
        {
            reader.close();
        }
        return sb.toString();
    }
}

Note that I've taken some shortcuts here: I've defined only one test, which is a positive test, covering all the different stylesheet rules. Ordinarily one would write both positive and negative tests for each individual rule, so that when a rule broke, one would immediately know exactly which rule it was. However, the all-in-one test is a little shorter, and thus easier to understand on a blog.

It is now possible to run this in one's IDE of choice and get that wonderful green bar, indicating the test has passed! So we can now begin to improve this code, which we will do in Part 2. Stay tuned!