Importers

A key characteristic of this system is that we need to be able to import documents of different types. For the purposes of this system you can rely on the files’ extensions in order to decide how to import them, since Dr. Avaj has been saving files with very specific extensions. All her letters have the .letter extension, reports have .report, and .jpg is the only image format used.

The simplest thing to do would be to just throw all the code for the importing mechanism into a single method, as shown in Example 4-1.

switch(extension) {
    case "letter":
        // code for importing letters.
        break;

    case "report":
        // code for importing reports.
        break;


    case "jpg":
        // code for importing images.
        break;

    default:
        throw new UnknownFileTypeException("For file: " + path);
}

This approach would have solved the problem in question but would be hard to extend. Every time you want to add another type of file that gets processed you would need to implement another entry in the switch statement. Over time this method would become intractably long and hard to read.

If you keep your main class nice and simple and split out different implementation classes for importing different types of documents, then it’s easy to locate and understand each importer in isolation. In order to support different document types, an Importer interface is defined. Each Importer will be a class that can import a different type of file.

Now that we know we need an interface to import the files, how should we represent the file that is going to be imported? We have a couple of different options: use a plain String to represent the path of the file, or use a class that represents a file, like java.io.File.

You could make the case that we should apply the principle of strong typing here: take a type that represents the file and reduce the scope for errors versus using a String. Let’s take that approach and use a java.io.File object as the parameter in our Importer interface to represent the file being imported, as shown in Example 4-2.

interface Importer {
    Document importFile(File file) throws IOException;
}

You might be asking, Why don’t you use a File for the public API of DocumentManagementSystem as well then? Well, in the case of this application, our public API would probably be wrapped up in some kind of user interface, and we aren’t sure what form that is taking files in. As a result we kept things simple and just used a String type.

The Document Class

Let’s also define the Document class at this point in time. Each document will have multiple attributes that we can search on. Different documents have different types of attributes. We have several different options that we can consider the pros and cons of when defining the Document.

The first and simplest way to represent a document would be to use a Map<String, String>, which is a map from attribute names to values associated with those attributes. So why not just pass a Map<String, String> around through the application? Well, introducing a domain class to model a single document is not just drinking the OOP Koolaid, but also provides a series of practical improvements in application maintability and readability.

For a start, the value of giving concrete names to components within an application cannot be overstated. Communication is King! Good teams of software developers use a Ubiquitous Language to describe their software. Matching the vocabulary that you use within the code of your application to the vocabulary that you use to talk to clients like Dr. Avaj makes things a lot easier to maintain. When you have a conversation with a colleague or client you will invariably need to agree upon some common language with which to describe different aspects of the software. By mapping this to the code itself, it makes it really easy to know what part of the code to change. This is called discoverability.

Another principle that should encourage you to introduce a class to model a document is strong typing. Many people use this term to refer to the nature of a programming language, but here we’re talking about the more practical use of strong typing in implementing your software. Types allow us to restrict the way in which data is used. For example, our Document class is immutable: once it has been created you can’t change, or mutate, any of its attributes. Our Importer implementations create the documents; nothing else modifies them. If you ever see a Document with an error in one of its attributes, you can narrow the source of the bug down to the specific Importer that created the Document. You can also infer from the immutability that it’s possible to index or cache any information associated with the Document and you know that it will be correct forever, since documents are immutable.

Another design choice that developers might consider when modeling their Document would be make the Document extend HashMap<String, String>. At first that seems great because the HashMap has all the functionality you need to model a Document. However, there are several reasons why this is a bad choice.

Software design is often as much about restricting functionality that is undesirable as it is about building things that you do want. We would have instantly thrown away the aforementioned benefits from immutability by allowing anything in the application to modify the Document class if it were just a subclass of HashMap. Wrapping the collection also gives us an opportunity to give more meaningful names to the methods, instead of, for example, looking up an attribute by calling the get() method, which doesn’t really mean anything! Later on we’ll go into more detail about inheritance versus composition, because this is really a specific example of that discussion.

In short, domain classes allow us to name a concept and restrict the possible behaviors and values of this concept in order to improve discoverability and reduce the scope for bugs. As a result, we’ve chosen to model the Document as shown in Example 4-3. If you’re wondering why it isn’t public like most interfaces, this is discussed later in “Scoping and Encapsulation Choices”.

public class Document {
    private final Map<String, String> attributes;

    Document(final Map<String, String> attributes) {
        this.attributes = attributes;
    }

    public String getAttribute(final String attributeName) {
        return attributes.get(attributeName);
    }
}

One final thing to note about Document is that it has a package-scoped constructor. Often Java classes make their constructor public, but this can be a bad choice as it allows code anywhere in your project to create objects of that type. Only code in the Document Management System should be able to create Documents, so we keep the constructor package scoped and restrict access to only the package that the Document Management System lives in.

Attributes and Hierarchical Documents

In our Document class we used Strings for attributes. Doesn’t this go against the principle of strong typing? The answer here is yes and no. We are storing attributes as text so that they can be searched through a text-based search. Not only that, but we want to ensure that all attributes are created in a very generic form that is independent of the Importer that created them. Strings aren’t a bad choice as such in this context. It should be noted that passing Strings around throughout an application in order to represent information is often considered a bad idea. In contrast with something being strongly typed, this is termed stringly typed!

In particular, if more complicated use was being made of the attribute values, then having different attribute types parsed out would be useful. For example, if we wanted to be able to find addresses within a certain distance or images with a height and width less than a certain size, then having strongly typed attributes would be a boon. It would be a lot easier to make comparisons with a width value that is an integer. In the case of this Document Management System, however, we simply don’t need that functionality.

You could design the Document Management System with a class hierarchy for Documents that models the Importer hierarchy. For example, a ReportImporter imports instances of the Report class that extends the Document class. This passes our basic sanity check for subclassing. In other words, it allows you to say a Report is a Document and it makes sense as a sentence. We chose not to go down that direction, however, as the right way to model classes in an OOP setting is to think in terms of behavior and data.

The documents are all modeled very generically in terms of named attributes, rather than specific fields that exist within different subclasses. Additionally, as far as this system is concerned, documents have very little behavior associated with them. There was simply no point in adding a class hierarchy here when it provided no benefit. You might think that this statement in and of itself is a little arbitrary, but it informs us of another principle: KISS.

You learned about the KISS principle in Chapter 2. KISS means that designs are better if they are kept simple. It’s often very hard to avoid unnecessary complexity, but it’s worth trying hard to do so. Whenever someone says, “we might need X” or “it would be cool if we also did Y,” just say No. Bloated and complex designs are paved with good intentions around extensibility and code that is a nice-to-have rather than must-have.

Implementing and Registering Importers

You can implement the Importer interface to look up different types of files. Example 4-4 shows the way that images are imported. One of the great things about Java’s core library is that it provides a lot of built-in functionality right out of the box. Here we read an image file using the ImageIO.read method and then extract the width and height of the image from the resulting BufferedImage object.

import static com.iteratrlearning.shu_book.chapter_04.Attributes.*;

class ImageImporter implements Importer {
    @Override
    public Document importFile(final File file) throws IOException {
        final Map<String, String> attributes = new HashMap<>();
        attributes.put(PATH, file.getPath());

        final BufferedImage image = ImageIO.read(file);
        attributes.put(WIDTH, String.valueOf(image.getWidth()));
        attributes.put(HEIGHT, String.valueOf(image.getHeight()));
        attributes.put(TYPE, "IMAGE");

        return new Document(attributes);
    }
}

Attribute names are constants defined in the Attributes class. This avoids bugs where different importers end up using different strings for the same attribute name; for example, "Path" versus "path". Java itself doesn’t have a direct concept of a constant as such, Example 4-5 shows the commonly used idiom. This constant is public because we want to be able to use it from different importers, though you may well have a private or package scoped constant instead. The use of the final keyword ensures that it can’t be reassigned to and static ensures that there is only a single instance per class.

public static final String PATH = "path";

There are importers for all three different types of files and you will see the other two implemented in “Extending and Reusing Code”. Don’t worry, we’re not hiding anything up our sleeves. In order to be able to use the Importer classes when we import files, we also need to register the importers to look them up. We use the extension of the file that we want to import as the key of the Map, as shown in Example 4-6.

    private final Map<String, Importer> extensionToImporter = new HashMap<>();

    public DocumentManagementSystem() {
        extensionToImporter.put("letter", new LetterImporter());
        extensionToImporter.put("report", new ReportImporter());
        extensionToImporter.put("jpg", new ImageImporter());
    }

Now that you know how to import documents, we can implement search. We won’t be focusing on the most efficient way to implement searching of documents here since we’re not trying to implement Google, just get the information to Dr. Avaj that she requires. A conversation with Dr. Avaj revealed that she wanted to be able to look up information about different attributes of a Document.

Her requirements could be met by just being able to find subsequences within attribute values. For example, she might want to search for documents that have a patient called Joe, and with Diet Coke in the body. We thus devised a very simple query language that consisted of a series of attribute name and substring pairs separated by commas. Our aforementioned query would be written as "patient:Joe,body:Diet Coke".

Since the search implementation keeps things simple rather than trying to be highly optimized, it just does a linear scan over all the documents recorded in the system and tests each one against the query. The query String that is passed to the search method is parsed into a Query object that can then be tested against each Document.

Making Importer a Class

You could have chosen to make a class hierarchy for importers, and have a class at the top for the Importer rather than an interface. Interfaces and classes provide a different set of capabilities. You can implement multiple interfaces, while classes can contain instance fields and it’s more usual to have method bodies in classes.

In this case the reason to have a hierarchy is to enable different importers to be used. You’ve already heard about our motivation for avoiding brittle class-based inheritance relationships, so it should be pretty clear that using interfaces is a better choice here.

That’s not to say that classes wouldn’t be a better choice elsewhere. If you want to model a strong is a relationship in your problem domain that involves state or a lot of behavior, then class-based inheritance is more appropriate. It’s just not the choice we think is most appropriate here.

Scoping and Encapsulation Choices

If you have taken the time to peruse the code you might notice that the Importer interface, its implementations, and our Query class are all package scoped. Package scope is the default scope, so if you see a class file with class Query at the top you know it’s package scoped, and if it says public class Query it’s public scoped. Package scoping means that other classes within the same package can see or have access to the class, but no one else can. It’s a cloaking device.

A strange thing about the Java ecosystem is that even though package scope is the default scope, whenever we’ve been involved in software development projects there are always more public-scoped classes than package-scoped ones. Perhaps the default should have been public all along, but either way package scope is a really useful tool. It helps you encapsulate these kinds of design decisions. A lot of this section has commented on the different choices that are available to you around designing the system, and you may want to refactor to one of these alternative designs when maintaining the system. This would be harder if we leaked details about this implementation outside of the package in question. Through diligent use of package scoping you can stop classes outside of the package making so many assumptions about that internal design.

We think it’s also worth reiterating that this is simply a justification and explanation of these design choices. There’s nothing inherently wrong with making other choices listed in this section—they may work out to be more appropriate depending on how the application evolves over time.

Test Naming

The first thing to think about when it comes to tests is their naming. Developers can get highly opinionated about naming—it’s an easy topic to talk about a lot because everyone can relate to it and think about the problem. We think the thing to remember is that there’s rarely a clear, really good name for something, but there are many, many, bad names.

The first test we wrote for the Document Management System was testing that we import a file and create a Document. This was written before we had introduced the concept of an Importer and weren’t testing Document-specific attributes. The code is in Example 4-16.

    @Test
    public void shouldImportFile() throws Exception
    {
        system.importFile(LETTER);

        final Document document = onlyDocument();

        assertAttributeEquals(document, Attributes.PATH, LETTER);
    }

This test was named shouldImportFile. The key driving principles when it comes to test naming are readability, maintainability, and acting as executable documentation. When you see a report of a test class being run, the names should act as statements that document what functionality works and what does not. This allows a developer to easily map from application behavior to a test that asserts that this behavior is implemented. By reducing the impedence mismatch between behavior and code, we make it easier for other developers to understand what is happening in the future. This is a test that confirms that the document management system imports a file.

There are lots of naming anti-patterns, however. The worst anti-pattern is to name a test something completely nondescript—for example, test1. What on earth is test1 testing? The reader’s patience? Treat people who are reading your code like you would like them to treat you.

Another common test naming anti-pattern is just named after a concept or a noun—for example, file or document. Test names should describe the behavior under test, not a concept. Another test naming anti-pattern is to simply name the test after a method that is invoked during testing, rather than the behavior. In this case the test might be named importFile.

You might ask, by naming our test shouldImportFile haven’t we committed this sin here? There’s some merit to the accusation, but here we’re just describing the behavior under test. In fact, the importFile method is tested by various tests; for example, shouldImportLetterAttributes, shouldImportReportAttributes, and shouldImportImageAttributes. None of those tests are called importFile—they are all describing more specific behaviors.

OK, now you know what bad naming looks like, so what is good test naming? You should follow three rules of thumb and use them to drive test naming:

Use domain terminology

Align the vocabulary used in your test names with that used when describing the problem domain or referred by the application itself.

Use natural language

Every test name should be something that you can easily read as a sentence. It should always describe some behavior in a readable way.

Be descriptive

Code will be read many times more often than it is written. Don’t skimp on spending more time thinking of a good name that’s descriptive up front and easier to understand later down the line. If you can’t think of a good name, why not ask a colleague? In golf, you win by putting in the fewest shots. Programming isn’t like that; shortest isn’t necessarily best.

You can follow the convention used in the DocumentManagementSystemTest of prefixing test names with the word “should,” or choose not to; that’s merely a matter of personal preference.

Behavior Not Implementation

If you’re writing a test for a class, a component, or even a system, then you should only be testing the public behavior of whatever is being tested. In the case of the Document Management System, we only have tests for the behavior of our public API in the form of DocumentManagementSystemTest. In this test we test the public API of the DocumentManagementSystem class and thus the whole system. The API can be seen in Example 4-17.

public class DocumentManagementSystem
{
    public void importFile(final String path) {
        ...
    }

    public List<Document> contents() {
        ...
    }

    public List<Document> search(final String query) {
        ...
    }
}

Our tests should only invoke these public API methods and not try to inspect the internal state of the objects or the design. This is one of the key mistakes made by developers that leads to hard-to-maintain tests. Relying on specific implementation details results in brittle tests because if you change the implementation detail in question, the test can start to fail even if the behavior is still working. Take a look at the test in Example 4-18.

    @Test
    public void shouldImportLetterAttributes() throws Exception
    {
        system.importFile(LETTER);

        final Document document = onlyDocument();

        assertAttributeEquals(document, PATIENT, JOE_BLOGGS);
        assertAttributeEquals(document, ADDRESS,
            "123 Fake Street
" +
                "Westminster
" +
                "London
" +
                "United Kingdom");
        assertAttributeEquals(document, BODY,
            "We are writing to you to confirm the re-scheduling of your appointment
" +
            "with Dr. Avaj from 29th December 2016 to 5th January 2017.");
        assertTypeIs("LETTER", document);
    }

One way of testing this letter-importing functionality would have been to write the test as a unit test on the LetterImporter class. This would have looked fairly similar: importing an example file and then making an assert about the result returned from the importer. In our tests, though, the mere existence of the LetterImporter is an implementation detail. In “Extending and Reusing Code”, you saw numerous other alternative choices for laying out our importer code. By laying out our tests in this manner, we give ourselves the choice to refactor our internals to a different design without breaking our tests.

So we’ve said that relying on the behavior of a class relies on using the public API, but there’s also some parts of the behavior that aren’t usually restricted just through making methods public or private. For example, we might not want to rely on the order of documents being being returned from the contents() method. That isn’t a property that’s restricted by the public API of the DocumentManagementSystem class, but simply something that you need to be careful to avoid doing.

A common anti-pattern in this regard is exposing otherwise private state through a getter or setter in order to make testing easier. You should try to avoid doing this wherever possible as it makes your tests brittle. If you have exposed this state to make testing superficially easier, then you end up making maintaining your application harder in the long run. This is because any change to your codebase that involves changing the way this internal state is represented now also requires altering your tests. This is sometimes a good indication that you need to refactor out a new class that can be more easily and effectively tested.

Don’t Repeat Yourself

“Extending and Reusing Code” extensively discusses how we can remove duplicate code from our application and where to place the resulting code. The exact same reasoning around maintenance applies equally to test code. Sadly, developers often simply don’t bother to remove duplication from tests in the same way as they would for application code. If you take a look at Example 4-19 you’ll see a test that repeatedly makes asserts about the different attributes that a resulting Document has.

    @Test
    public void shouldImportImageAttributes() throws Exception
    {
        system.importFile(XRAY);

        final Document document = onlyDocument();

        assertAttributeEquals(document, WIDTH, "320");
        assertAttributeEquals(document, HEIGHT, "179");
        assertTypeIs("IMAGE", document);
    }

Normally you would have to look up the attribute name for every attribute and assert that it is equal to an expected value. In the case of the tests here, this is a common enough operation that a common method, assertAttributeEquals, was extracted with this logic. Its implementation is shown in Example 4-20.

    private void assertAttributeEquals(
        final Document document,
        final String attributeName,
        final String expectedValue)
    {
        assertEquals(
            "Document has the wrong value for " + attributeName,
            expectedValue,
            document.getAttribute(attributeName));
    }

Good Diagnostics

Tests would be no good if they didn’t fail. In fact, if you’ve never seen a test fail how do you know if it’s working at all? When writing tests the best thing to do is to optimize for failure. When we say optimize, we don’t mean make the test run faster when it fails—we mean ensure that it is written in a way that makes understanding why and how it failed as easy as possible. The trick to this is good diagnostics.

By diagnostics we mean the message and information that gets printed out when a test fails. The clearer this message is about what has failed, the easier it is to debug the test failure. You might ask why even bother with this when a lot of the time Java tests are run from within modern IDEs that have debuggers built in? Well, sometimes tests may be run within continuous integration environments, and sometimes they may be from the command line. Even if you’re running them within an IDE it is still helpful to have good diagnostic information. Hopefully, we’ve convinced you of the need for good diagnostics, but what do they look like in code?

Example 4-21 shows a method that asserts that the system only contains a single document. We will explain the hasSize() method in a little bit.

    private Document onlyDocument()
    {
        final List<Document> documents = system.contents();
        assertThat(documents, hasSize(1));
        return documents.get(0);
    }

The simplest type of assert that JUnit offers us is assertTrue(), which will take a boolean value that it expects to be true. Example 4-22 shows how we could have just used assertTrue to implement the test. In this case the value is being checked to equal 0 so that it will fail the shouldImportFile test and thus demonstrate the failure diagnostics. The problem with this is that we don’t get very good diagnostics—just an AssertionError with no information in the message shown in Figure 4-1. You don’t know what failed, and you don’t know what values were being compared. You know nothing, even if your name isn’t Jon Snow.

assertTrue(documents.size() == 0);

Figure 4-1. Screenshot of assertTrue failing

The most commonly used assertion is assertEquals, which takes two values and checks they are equal and is overloaded to support primitive values. So here we can assert that the size of the documents list is 0, as shown in Example 4-23. This produces a slightly better diagnostic as shown in Figure 4-2, you know that the expected value was 0 and the actual value was 1, but it still doesn’t give you any meaningful context.

assertEquals(0, documents.size());

Figure 4-2. Screenshot of assertEquals example failing

The best way of making an assert about the size itself is to use a matcher for asserting the collection size as this provides the most descriptive diagnostics. Example 4-24 has our example written in that style and demonstrates the output as well. As Figure 4-3 shows, this is much clearer as to what went wrong without you needing to write any more code.

assertThat(documents, hasSize(0));

Figure 4-3. Screenshot of assertThat example failing

What is going on here is that JUnit’s assertThat() is being used. The method assertThat() takes a value as its first parameter and a Matcher as its second. The Matcher encapsulates the concept of whether a value matches some property and also its associated diagnostics. The hasSize matcher is statically imported from a Matchers utility class that contains a bundle of different matchers and checks that the size of a collection is equal to its parameter. These matchers come from the Hamcrest library, which is a very commonly used Java library that enables cleaner testing.

Another example of how you can build better diagnostics was shown in Example 4-20. Here an assertEquals would have given us the diagnostic for the attribute’s expected value and actual value. It wouldn’t have told us what the name of the attribute was, so this was added into the message string to help us understand failure.

Testing Error Cases

One of the absolute worst and most common mistakes to make when writing software is only to test the beautiful, golden, happy path of your application—the code path that is executed when the sun is shining on you and nothing goes wrong. In practice lots of things can go wrong! If you don’t test how your application behaves in these situations, you’re not going to end up with software that will work reliably in a production setting.

When it comes to importing documents into our Document Management System there are a couple of error cases that might happen. We might try to import a file that doesn’t exist or can’t be read, or we might try to import a file that we don’t know how to extract text from or read.

Our DocumentManagementSystemTest has a couple of tests, shown in Example 4-25, that test these two scenarios. In both cases we try to import a path file that will expose the problem. In order to make an assert about the desired behavior we use the expected = attribute of JUnit’s @Test annotation. This enables you to say Hey listen, JUnit, I’m expecting this test to throw an exception, it’s of a certain type.

    @Test(expected = FileNotFoundException.class)
    public void shouldNotImportMissingFile() throws Exception
    {
        system.importFile("gobbledygook.txt");
    }

    @Test(expected = UnknownFileTypeException.class)
    public void shouldNotImportUnknownFile() throws Exception
    {
        system.importFile(RESOURCES + "unknown.txt");
    }

You may want an alternative behavior to simply throwing an exception in the case of an error, but it’s definitely helpful to know how to assert that an exception is thrown.

Constants

Constants are values that do not change. Let’s face it—they are one of the few well-named concepts when it comes to computer programming. The Java programming language doesn’t use an explicit const keyword like C++ does, but conventionally developers create static field fields in order to represent constants. Since many tests consist of examples of how a part of your computer program should be used, they often consist of many constants.

It’s a good idea when it comes to constants that have some kind of nonobvious meaning to give them a proper name that can be used within tests. We do that extensively through the DocumentManagementSystemTest, and in fact, have a block at the top dedicated to declaring constants, shown in Example 4-26.

public class DocumentManagementSystemTest
{
    private static final String RESOURCES =
        "src" + File.separator + "test" + File.separator + "resources" + File.separator;
    private static final String LETTER = RESOURCES + "patient.letter";
    private static final String REPORT = RESOURCES + "patient.report";
    private static final String XRAY = RESOURCES + "xray.jpg";
    private static final String INVOICE = RESOURCES + "patient.invoice";
    private static final String JOE_BLOGGS = "Joe Bloggs";