Keep your code clean

Keeping the code clean is not as easy that you first may assume. I have seen code examples that make you shiver but still it’s hard to really remove all the noise without changing the fundamental behaviour.

So lets begin with an example (in java):

public ValueObject messyFunction() throws CustomException {
    ValueObject vo = new ValueObject();
    
    try {
        // So first we need to fill some information in our object
        // This will call the server and fill some information in the object.
        // If this call fails it will throw a CustomException
        service.getValueObjectInformation(vo);
    
        // Read all information from the file
        // If the file don't exist it will throw a FileNotFoundException
        // If we fail to read it will throw an IOException
        FileReader fr = new FileReader(Constants.FILE_TO_USE);
        if (vo != null) {
            String line = null;
            do {
                // Iterate over the lines of a file
                // Skip null to avoid null pointer exceptions
                if ((line != null) && (line != "")) {
                    // Add the line to the value object
                    // This is because we want to use it later.
                    vo.addData(line);
                }
                // Read the next line from the file.
                // This may throw an IOException
                // This will return null if the last line has been reached.
            } while((line = fr.readLine()) != null);
        }
        fr.close();
    } catch (IOException e) {
        // TODO: Auto generated catch block.
        e.printStacktrace();
    } catch (CustomException e) {
        throw e;
    } catch (Throwable t) {
        log.info(vo, t);
        throw new CustomException(t);
    }
    return vo;
}

So this is readable and well commented at first sight. But lets analyse it in more detail.

First the comments, what exactly are the value over the code in them? It seems like it was written in educational purpose (doh =), but I have actual seen similar constructions in production code). It’s more or less stating the obvious, you might expect that the developer has some knowledge of the system so the service call will generate a server call. The method declaration states that it might throw the exception. So the description of the call should therefore be described in the javadoc for the called method.

Then we will create a file reader. Again stating things that is defined in the javadoc in the code. So this is again useless information. The following comments follows the same pattern. So it’s recommended to remove all comments.

Then lets see what we are catching, IOException, CustomException and Throwable. Way catching a Throwable? This will actually hides unchecked exceptions like null pointer exceptions and array out of bounds. In this example one can suspect that the service is forgotten to be instantiated, this will then be hidden in the catch Throwable and wrapped in CustomException. So finding that bug is much harder then not catch the exception.

Also logging the vo and printing the IOException to standard error seams like it’s not normalized what to do.

The loop and null check is also wired, by definition the ValueObject can’t be null sins it’s instantiated in the method, and the service can’t null the parameter. So that null check is not needed. The loop is backwards so we can skip the null check of line if we do a ordinary while loop.

Then if we look at the file reading it is a potential resource leak, if we got an exception during the reading it will not be closed. This can be solved with try-with-resources in Java 7 or by using a finally block in earlier version of Java.

So after some refactoring and clean-up we can create a function that looks like this:

public ValueObject notSoMessyFunction() throws CustomException {
    ValueObject vo = new ValueObject();
    
    service.getValueObjectInformation(vo);
    
    try (FileReader fr = new FileReader(Constants.FILE_TO_USE)) {
        String line;
        while((line = fr.readLine()) != null) {
            if (!line.trim().isEmpty())) {
                vo.addData(line);
            }
        } 
    } catch (IOException e) {
        log.info(vo, e);
        // Return as much information as possible so don't throw here
    }
    return vo;
}

There is still some things that can be done, like filling a ValueObject(?), shouldn’t that be an immutable object?. Checked exceptions can we work around that in some way? Also it needs some more refactoring to make the code really clean.

This can be discussed in later post.