[PATCH] Make overriding `DownloadOsmTask.AbstractInternalTask.createNewLayer()` methods easier

[floscher]

The issue

At ​JOSM/geojson issue #24 (comment) the method DownloadOsmTask.AbstractInternalTask.createNewLayer(String) is overridden and returns a subclass of OsmDataLayer. To do this, the whole logic had to be duplicated from the "super" method.

What the patch changes

My patch creates a new method (createNewLayer(DataSet, String)), which only instantiates the OsmDataLayer object. This can be easily overridden by plugins to return instances of any subclass.

I also added //TODO comments to make createNewLayer() and createNewLayer(String) final (currently these are overridden by plugins cadastre-fr, geojson, opendata). Since these are only convenience methods that eventually call createNewLayer(DataSet, String), they should not be overridable. When plugins no longer override the methods, they should be made final to enforce this.

The new method getNonBlankLayerName(String) is created so subclasses can change how the layer name is determined. It takes a nullable string and returns a non-blank (not null, length >= 1 and at least one non-whitespace char) layer name.

[michael2402]

getNonBlankLayerName has a very bad logic.

I would prefer to remove the parameter and then use the following logic:

protected OsmDataLayer createNewLayer(final DataSet dataset, final Optional<String> layerName) {
     Objects.requireNonNull(dataset, "dataset");
     Objects.requireNonNull(layerName, "layerName");
     return new OsmDataLayer(dataset, layerName.orElseGet(this::getNonBlankLayerName), null);
}

... createNewLayer(String layerName) {
    var optionalLayerName = Optional.ofNullable(layerName).filter(name -> !name.isEmpty())
    ...
}

[floscher]

@michael2402: Ah yes, thanks. I'll have another look how I can improve the patch.

[floscher]

I now modified the patch to work with Optionals in getNonBlankLayerName(), also I remembered now that ​.trim().isEmpty() is inefficient. The parameter of createNewLayer() is still a nullable String, there it shouldn't matter much if an Optional is passed through the convenience methods or a nullable string.

By the way: I have patches for the plugins, which I'll propose after this is merged.

[michael2402]

@floscher

The thing we should change in my opinion is the signature of the method createNewLayer. I know that the old code passes a string that can be null. We use nullable/empty Strings a lot in JOSM since there was no other way to mark optional parameters in pre-Java8 code.

In newer code, we should try to move away from the implicit "if null or empty do..." semantics and explicitly ask the users of our methods to specify wether they want to use their own value or they want us to get a default one.

I personally would write:

/**
 * @param dataset The dataset for the new layer
 * @param layerName The name of the new layer. If empty, a default name will be generated
 */
protected final OsmDataLayer createNewLayer(final DataSet dataset, final Optional<String> layerName) {
// if layerName is null: Throw NPE
// if layerName is present and empty: Throw IllegalArgumentException
//...

   return createNewLayer(dataset, computedLayerName);
}

/**
 * Create a new layer
 * @param dataset The dataset for the new layer
 * @param layerName The name of the new layer, never null
 */
protected OsmDataLayer createNewLayer(final DataSet dataset, final String layerName) {
   // ...
}

Using those semantics, it is clear that getNonBlankLayerName should not be called if the layerName was explicitly set and that if a layerName was explicitly set, we can assume that the new layer has that name.

getNonBlankLayerName can then be renamed to "generateLayerName()"

[floscher]

Ah, now I think I understand. And thanks for the name suggestion, I was searching for a better name for that method but couldn't think of one.

But I'd change the parameter type of createNewLayer(String) to createNewLayer(Optional<String>) and remove createNewLayer() later when making that method also final. That way the plugins using them don't break.

[floscher]

Another update

[floscher]

Instead of TODO comments, deprecate old API and already add new API

[Don-vip]

In 14347/josm:

fix #16850 - Make overriding DownloadOsmTask.AbstractInternalTask.createNewLayer() methods easier (patch by floscher, modified)

[Don-vip]

In 14350/josm:

see #16850 - fix error_prone warning