Defensive coding: Null-safe string comparisons

One of the most valuable tips I received why having my Java code reviewed was to change this code:

if (merchant.getSalesChannel().equals("Mexico")) {
    …
}

to this:

if ("Mexico".equals(merchant.getSalesChannel()) {
    …
}

Everything in Java is an object. Thus, a simple string literal immediately creates a new object. We are sure that a literal is not null.

The other part of the equation might surprise us with a null. Who knows what that getter might return? Most likely it is some value from the database. Are we sure that the value was properly retrieved? Are we sure there’s a NOT NULL constraint on the database column? We can never be sure.

So if we want to compare a variable to a string literal, let’s use the null-safe style proposed in the second listing above.

Further reading:

https://stackoverflow.com/questions/43409500/compare-strings-avoiding-nullpointerexception/43409501

https://softwareengineering.stackexchange.com/questions/313673/is-use-abc-equalsmystring-instead-of-mystring-equalsabc-to-avoid-null-p

https://www.javacodegeeks.com/2012/06/avoid-null-pointer-exception-in-java.html

Defensive coding: Make local objects final

In the very first article on defensive coding we talked about avoiding mutability.

Let’s talk about JavaScript for a while. Every modern tutorial says that you can instantiate new objects either with let or const. You use let if you intend to create a variable (like with an obsolete var keyword), or you can use const to indicate that an object – once initialized – should remain constant. This gives developers great clarity and saves them from mistaken mutations.

What about Java? You don’t have a const keyword, but you have final. I love using it and many developers in my current project also use final wherever feasible. You can mark properties, local objects and method arguments as final. There are very rare cases when we need to mutate an object, so must of the time I end up having final everywhere:

public BigDecimal getTotalPrice(final Product product,
                                final BigInteger quantity) {
    final BigDecimal totalNetPrice = product
             .getUnitPrice()
             .multiply(quantity);

    return totalNetPrice.multiply(product.getTaxRate());
}

Why use final everywhere? It can look strange in the beginning, but it protects us from bugs. How many times have you seen robust methods spanning multiple screens, with vague and similar variable names? How many times have you mistaken foo, oldFoo, databaseFoo, foo1 etc.?

In PHP, the final keyword can be applied only to classes. You can’t create local constants, only class constants with const keyword. It’s even worse that you don’t clearly distingish first object instantiation from subsequent instantations using the same variable name. This code is perfectly valid in PHP:

$builder = new ProductBuilder();
$builder = new UserBuilder();

Defensive coding: Final properties and proper autowiring in Spring

How to get rid of “Field injection is not recommended” warning (and what’s the problem)?

I work in a project where most dependencies in Spring are declared this way:

@Component
class DocumentsController {
    @Autowired
    private UsersRepository users;

    @Autowired
    private DocumentsRepository documents;

    @Autowired
    private PdfGenerator pdfGenerator;
}

For every @Autowired annotation above, my favorite IDE complains that “field injection is not recommended”. What does it mean?

It’s a nasty hack. I believe every class should be explicit about its dependencies, especially the required ones. They should go into a constructor. Anyone trying to instantiate above class manually should immediately see what is needed.

Dependencies should not be mutable. In the example above, you cannot use final for properties because your compiler will complain that the objects are not initialized. The compiler doesn’t know the Spring magic behind instantation of these objects. This means that someone can accidentally overwrite the property.

Let’s refactor that code:

@Component
final class DocumentsController {
    private final UsersRepository users;
    private final DocumentsRepository documents;
    private final PdfGenerator pdfGenerator;

    @Autowired
    public DocumentsController(final UsersRepository users,
                               final DocumentsRepository documents,
                               final PdfGenerator pdfGenerator) {
        this.users = users;
        this.documents = documents;
        this.pdfGenerator = pdfGenerator;
    }
}

If you don’t want to write boilerplate code, you can use Lombok’s @RequiredArgsConstructor to create the constructor automatically. Still, you benefit from using final and having explicit dependencies (but this requires having a Lombok plugin for your IDE).

Testing PDF documents

I’ve been wondering for some time if PDF is still a valid format. It’s “portable”, of course, but not in today’s meaning – it’s clearly not responsive. Like a fixed piece of paper transformed into a file. However, PDF still has many important use cases like storing invoices, reports or tickets. I spent a couple of years working on sophisticated PDF reports, and this year I even tried to test a process of generating invoices in some ad exchange system. I really wanted this system to be rock solid.

Of course there is no point in comparing a binary PDF file to an expected value. You can’t catch the exact differences in case of an error. I could create a PNG screenshot and compare it to the template, but I was a bit worried about the readability of such diff. A third way would be to verify the source HTML document used to render a PDF – but in fact, I was not interested in markup, but in an output data that landed inside a PDF.

Following my friend’s advice, I used another tool called Apache PDFBox. This robust library allows performing different operations on PDF documents: creating, merging, splitting, signing, filling forms etc. We decided to extract plain text from a file. It’s like we used a Select all command, copy and paste the text into Notepad.

PDDocument pdf = PDDocument.load(content);
PDFTextStripper stripper = new PDFTextStripper();
stripper.setAddMoreFormatting(true);
stripper.setSortByPosition(true);
stripper.setStartPage(0);
stripper.setEndPage(pdf.getNumberOfPages());
String plainText = stripper.getText(pdf);
pdf.close();

assertThat(plainText).isEqualTo(expected);

A PDF document consists of blocks which can be ordered in a way that we did not really expect. Anyone who tried copying a table from PDF to Notepad experienced this. Luckily, PDFBox tries to help us organizing the blocks and formatting the plain text dump.

We made a lot of test scenarios using the above solution and they did a really good job catching all the little bugs in the data. It was crucial to detect any mistakes because our system was preparing financial documents. Moreover, the test reports were very readable.

The only problem with the above method is that it does not test the layout correctness. To achieve that, we could extract only specified regions from the document. In such case we assume that a rectangle with x1,y1,x2,y2 coordinates contain, for example, customer’s data:

Rectangle2D region = new Rectangle2D.Double(x1, y1, x2 - x1, y2 - y1);
PDDocument pdf = PDDocument.load(content);
PDFTextStripperByArea stripper = new PDFTextStripperByArea();
stripper.addRegion("Contact region", region);
stripper.extractRegions(pdf.getPage(0));
String plainText = stripper.getTextForRegion("Contact region");
pdf.close();

assertThat(plainText).isEqualTo(expected);

Java: Integer or int?

When I first saw primitive types like int or boolean mixed up with classes, I was very tempted to convert all primitives into IntegerBoolean and so on to maintain a clear coding style. But reading articles on the Internet and IntelliJ hints stopped me from doing such a stupid thing.

I read this wonderful rule of thumb:

Don’t create unnecessary objects.

Primivites always occupy less memory than objects, which was clearly described in this article on primitives. Maybe we don’t usually care about the memory usage, but if we process big amounts of data, every byte can count. Moreover, accessing primitives is faster because they are stored on the stack, not on the heap.

It’s also important to remember that a variable pointing to an Integer or Boolean object can be null. A primitive can’t. This difference matters for example when you retrieve data from an SQL database which allows NULLs. If you try to assign null to a primitive, you’ll get a Null Pointer Exception. That’s a bug I noticed and fixed in a production system. So I always try to restrict the possibility to use NULLs in the database and in the code unless null has a real business meaning.

NPE: Converting a list of objects to a map

Like every Java developer, I had a fair amount of Null Pointer Exceptions in my career. That’s why I decided to describe real-world examples I had to fix.

Today I will show you how to get an NPE while retrieving records from a database and creating a map based on these records, using streams and collectors introduced in Java 8.

Let’s say we have a Setting class:

public class Setting {
private final int accountId;
private final String type;
private final String value;

public Setting(int accountId, String type, String value) {
this.accountId = accountId;
this.type = type;
this.value = value;
}

public String type() {
return type;
}

public String value() {
return value;
}
}

Now, in another part of the system we will retrieve Setting objects from a database. We haven’t noticed that the SQL table allows NULL in the value column. Let’s pretend our list of settings looks like that:

List<Setting> settings = new ArrayList<>();
settings.add(new Setting(1, "site_title", "My blog"));
settings.add(new Setting(1, "site_description", "A place with fresh ideas"));
settings.add(new Setting(1, "site_copyright", null));

We want to easily access settings by names, so we create a map using a stream:Ponieważ chcemy mieć łatwy dostęp do ustawień o konkretnej nazwie, tworzymy mapę za pomocą strumienia:

Map<String, String> settingsMap = settings
.stream()
.collect(Collectors.toMap(Setting::type, Setting::value));

Bang! We’ve got a NPE here because one of the values used to create a map is null. It turns out that Collectors.toMap() does not handle NULLs.

Let’s think: do we really need a NULL in this case? I guess not. A NULL value means “no data” or “unknown value”, which is the same as if the setting did not exist. We should set the value column as NOT NULL. We can additionally filter the settings list:

Map<String, String> settingsMap = settings
.stream()
.filter(setting -> null != setting.value())
.collect(Collectors.toMap(Setting::type, Setting::value));

A NULL would make sense for example if we wanted to create a map between people’s names and their ages. If we don’t know someone’s age, then a null seems reasonable. We would have then to create a map in a different way.

You can have a further read on StackOverflow. Alternative ways to convert a list to a map can be found on Baeldung.