Null values in Options Chains

[petermr]

Methods such as org.contentmine.ami.tools.AbstractAMITool.getCProjectDirectory()

protected String getCProjectDirectory() {
    return parent.projectOrTreeOptions.cProjectOptions.cProjectDirectory;
}

can crash on nulls:

Generic values (AMIFilterTool)
================================
java.lang.NullPointerException
    at org.contentmine.ami.tools.AbstractAMITool.getCProjectDirectory(AbstractAMITool.java:427)
    at org.contentmine.ami.tools.AbstractAMITool.validateCProject(AbstractAMITool.java:247)
    at org.contentmine.ami.tools.AbstractAMITool.parseGenerics(AbstractAMITool.java:219)
...

it's difficult to know which of the objects in the chain are null. (a) Is'nt there something in later Java Versions which helps detect where? (b) should we recurse down trapping each level? (c) is <Optional> useful here?

[remkop]

From looking at the code, this can only happen if parent is null. I am guessing this is happening in a test or in some other code that executes AMIFilterTool directly instead of AMI.execute(AMIFilterTool.class, args).

(a) Yes, by running on Java 14 we will get a more helpful NPE (we don't even need to compile with Java 14, but we might as well) (b) We could check each element, and return null if an element of the chain is null. Do we want to allow parent to be null for easier testing? (c) Optional has the advantage that it clearly signals to the caller that the result may be null.

I think it makes sense for methods like getCProjectDirectory and getCTreeDirectory to return Optional<String> to indicate that there may not be a value.

The way that the top-level options are organized makes the parent required which in turn makes testing less easy and less obvious. Is it an idea to make --cproject and --ctree optional and use the current directory as the default? Perhaps that would allow us to return a value from methods like getCProjectDirectory even if parent is null. (getCTreeDirectory would only return something if the --ctree option was actually specified.)

[remkop]

Perhaps the most robust approach is to assert for each component in the train that it is not null.

That is, replace:

return parent.projectOrTreeOptions.cTreeOptions.cTreeDirectory

with

import static java.util.Objects.requireNonNull;

...
return requireNonNull(
        requireNonNull(
                requireNonNull(parent, "parent")
                        .projectOrTreeOptions, "projectOrTreeOptions")
                .cTreeOptions, "cTreeOptions")
        .cTreeDirectory;

This will throw a NullPointerException that shows which component is null. Works on any version of Java greater than Java 7 or 8.

[remkop]

I made some changes to AbstractAMITool to implement the above. Also factored out some common call chains to separate methods:

• getParentGeneralOptions
• getParentCProjectOptions
• getParentCTreeOptions

These are private helper methods to reduce code duplication. They could be made protected if they are needed in subclasses, but for now private is fine.