RSS 2.0 Blog feedThis is a follow up to a previous post in which I described a refactoring of a piece of real-world code that took advantage of some Java 8 constructs. You should first refer to that post to understand the context of the discussion and a few hints to understand what's the purpose of the code I'm talking about.
Below there is the original chunk of code that has been refactored. Since it came after a previous refactoring
where Metadata was fitted with Optional, we see things such as if
(artist.isPresent()) in place of if (artist != null), but, as I stated in my previous post,
this kind of refactoring is unsatisfactory. There's a line in which a simple conditional has been replaced by Optional.orElse(),
but - again - it's just a little step forward. Let's see how it can be replaced by something better, possibly
totally erasing the five major conditional branches in the listing.
public void importFallbackTrackMetadata (final @Nonnull MediaItem mediaItem, final @Nonnull URI trackUri)
{
log.debug("importFallbackTrackMetadata({}, {})", mediaItem, trackUri);
final Metadata metadata = mediaItem.getMetadata();
StatementManager.Builder builder = statementManager.requestAddStatements();
final Optional<String> title = metadata.get(Metadata.TITLE);
if (artist.isPresent())
{
builder = builder.with(trackUri, DC.TITLE, literalFor(title))
.with(trackUri, RDFS.LABEL, literalFor(title));
}
final Optional<String> artist = metadata.get(Metadata.ARTIST);
URI artistUri = null;
if (artist.isPresent())
{
final String artistName = artist.get();
artistUri = localArtistUriFor(idCreator.createSha1("ARTIST:" + artistName));
builder = builder.with(trackUri, FOAF.MAKER, artistUri);
if (seenArtistUris.putIfAbsent(artistUri, true) == null)
{
final Value nameLiteral = literalFor(artistName);
builder = builder.with(artistUri, RDF.TYPE, MO.C_MUSIC_ARTIST)
.with(artistUri, FOAF.NAME, nameLiteral)
.with(artistUri, RDFS.LABEL, nameLiteral);
}
}
final MediaFolder parent = (MediaFolder)mediaItem.getParent();
final String recordTitle = metadata.get(Metadata.ALBUM).orElse(parent.getPath().toFile().getName());
final URI recordUri = localRecordUriFor(idCreator.createSha1("RECORD:" + recordTitle));
if (seenRecordUris.putIfAbsent(recordUri, true) == null)
{
final Value titleLiteral = literalFor(recordTitle);
builder = builder.with(recordUri, RDF.TYPE, MO.C_RECORD)
.with(recordUri, DC.TITLE, titleLiteral)
.with(recordUri, RDFS.LABEL, titleLiteral)
.with(recordUri, MO.P_MEDIA_TYPE, MO.C_CD);
if (artist.isPresent())
{
builder = builder.with(recordUri, FOAF.MAKER, artistUri);
}
}
builder.with(recordUri, MO.P_TRACK, trackUri)
.publish();
}
The logics of this piece of code is:
- given a musical track, retrieve its title and the name of the artist (both of them can be present or not);
- if the title is present, add two statements for it;
- if the artist is present, create an unique, local id for him, and associate him to the track;
- if it's the first time we see the artist, add some further statement to describe the artist;
- try to extract the title of the record that contains the track; if it's not present, the name of the folder containing the file will be used;
- if it's the first time we see the record, add some further statements to describe it;
- if both the artist and the record exist, add another statement to associate them;
- at last, add a statement to relate the track with the record.
It seems that the refactoring is tricky: not only because of the conditionals, but also because of the
side-effect of putIfAbsent(). I recall that this method of a ConcurrentMap is capable
of an atomic operation of putting a new key into a map only if it's not present (the atomicity makes synchronized
not needed and improves the performance). The method returns null if the key was actually inserted.
In the space of a short lunch break I was able to refactor the whole method into:
public void importFallbackTrackMetadata (final @Nonnull MediaItem mediaItem, final @Nonnull URI trackUri)
{
log.debug("importFallbackTrackMetadata({}, {})", mediaItem, trackUri);
final Metadata metadata = mediaItem.getMetadata();
final Optional<String> title = metadata.get(Metadata.TITLE);
final Optional<String> artistName = metadata.get(Metadata.ARTIST);
final MediaFolder parent = (MediaFolder)mediaItem.getParent();
final String recordTitle = metadata.get(Metadata.ALBUM).orElse(parent.getPath().toFile().getName());
final Optional<URI> artistUri = artistName.map(name -> localArtistUriFor(idCreator.createSha1("ARTIST:" + name)));
final URI recordUri = localRecordUriFor(idCreator.createSha1("RECORD:" + recordTitle));
final Optional<URI> newArtistUri = seenArtistUris.putIfAbsentAndGetNewKey(artistUri, true);
final Optional<URI> newRecordUri = seenRecordUris.putIfAbsentAndGetNewKey(recordUri, true);
statementManager.requestAddStatements()
.with(trackUri, DC.TITLE, literalFor(title))
.with(trackUri, RDFS.LABEL, literalFor(title))
.with(trackUri, FOAF.MAKER, artistUri)
.with(recordUri, MO.P_TRACK, trackUri)
.with(newArtistUri, RDF.TYPE, MO.C_MUSIC_ARTIST)
.with(newArtistUri, RDFS.LABEL, literalFor(artistName))
.with(newArtistUri, FOAF.NAME, literalFor(artistName))
.with(newRecordUri, RDF.TYPE, MO.C_RECORD)
.with(newRecordUri, RDFS.LABEL, literalFor(recordTitle))
.with(newRecordUri, DC.TITLE, literalFor(recordTitle))
.with(newRecordUri, MO.P_MEDIA_TYPE, MO.C_CD)
.with(newRecordUri, FOAF.MAKER, artistUri)
.publish();
}
Let's first discuss the qualities of the refactored method:
- it's shorter;
- five conditional branches have been removed and, apparently, there is less complexity to understand;
- it was possible to completely partition the method in three parts: first the retrieval of metadata, then the
test about whether the artist and record were already registered and at last the creation of all the statements.
In particular, the latter part made it possible to write a regular fluent builder section, without repeating
chunks of
builder = builder.with(...)all around.
The refactoring was essentially based on a single trick: replacing the conditionals about data items with Optional
wrapping those data items. Hence we have a Optional<URI> artistUri and recordUri.
But even the conditions “was the artist/record already inserted” were mapped into an Optional: in
fact, newArtistUri and newRecordUri might be not present (even though artistUri
is). In other words, I split the concepts “we have an artist id” / “we have a new artist id” into two seperate Optionals.
This came with a cost, that is adapting the existing facility methods and the builder for accepting Optional.
The facility method literalFor() has been overloaded to accept an Optional<String>
and return an Optional<Value>; if the string is not present, the value won't be present too:
@Nonnull
public static Optional<Value> literalFor (final Optional<String> optionalString)
{
return optionalString.map(string -> literalFor(string));
}
The new builder has got two overloaded methods, which accept Optional elements; if any element of a
subject-predicate-object triple is missing, no statement will be added:
@Nonnull
public Builder with (final @Nonnull Optional<? extends Resource> optionalSubject,
final @Nonnull URI predicate,
final @Nonnull Value object)
{
return optionalSubject.map(subject -> with(subject, predicate, object)).orElse(this);
}
@Nonnull
public Builder with (final @Nonnull Optional<? extends Resource> optionalSubject,
final @Nonnull URI predicate,
final @Nonnull Optional<? extends Value> optionalObject)
{
return optionalObject.map(object -> with(optionalSubject, predicate, object)).orElse(this);
}
At first I was a bit puzzled about how to deal with the two Optionals in the latter
method: map() resolves a single instance, and for a short time I was not able to think beyond
accepting a if (optionalSubject.isPresent() && optionalObject.isPresent()). The solution
is simple: reduction. Resolve one problem and delegate the resolution of the other to another method.
In other words, I would have introduced the former method with a single Optional even though
there was not a direct need in the original code.
Some further work needed to be done with ConcurrentMap, because this class hasn't been updated to
deal with Optional. So I created a specialisation:
public class ConcurrentHashMapWithOptionals<K, V> extends ConcurrentHashMap<K, V>
{
@Nonnull
public Optional<K> putIfAbsentAndGetNewKey (final @Nonnull Optional<K> optionalKey, final @Nonnull V value)
{
return optionalKey.flatMap(key -> putIfAbsentAndGetNewKey(key, value));
}
@Nonnull
public Optional<K> putIfAbsentAndGetNewKey (final @Nonnull K key, final @Nonnull V value)
{
return (putIfAbsent(key, value) == null) ? Optional.of(key) : Optional.empty();
}
}
Not only it accepts an Optional key, but it also returns an Optional value in place
of the nullable returned by putIfAbsent(). Also the semantics has been slightly changed (hence the
new name putIfAbsentAndGetNewKey()): it returns the current key (not the previous one) if it has
been added, otherwise an empty Optional.
In the end, I think that the refactoring was good.
As in the previous case, you save some lines and gain some readability in the main code, but at the expense of
some more lines of code behind the scenes. In this case, the main reason for this is the fact that not all the
utility classes in the Java runtime have been retrofitted for working with Optional; in other words,
we're filling a gap left by Oracle. On the other hand, the new class is totally reusable in similar cases.
Playing the devil's advocate, one might question whether the refactored code is really more readable
for everybody, including those not aware of the new features in Java 8. While a conditional is something that is
well understood by any programmer, and it immediately stands out while reading, in this case one might be
surprised by the fact that, while all the statement creations looks similar because they are all passed in methods
called with(...), some of them might be not created at all. It's a sort of violation of the Principle
of Least Astonishment applied to human understanding of code. Sure, reading Optional should be
a surprise for people who don't know Java 8 and push them to search for the documentation, but its presence might
be unnoticed during a skim through the code that focuses on the section where statements are created.
While I'd like to avoid a comment, because readable code shoundn't need comments, perhaps it suffices to make the
thing more evident by renaming the with() methods that accept an Optional into a withOptional():
public void importFallbackTrackMetadata (final @Nonnull MediaItem mediaItem, final @Nonnull URI trackUri)
{
log.debug("importFallbackTrackMetadata({}, {})", mediaItem, trackUri);
final Metadata metadata = mediaItem.getMetadata();
final Optional<String> title = metadata.get(Metadata.TITLE);
final Optional<String> artistName = metadata.get(Metadata.ARTIST);
final MediaFolder parent = (MediaFolder)mediaItem.getParent();
final String recordTitle = metadata.get(Metadata.ALBUM).orElse(parent.getPath().toFile().getName());
final Optional<URI> artistUri = artistName.map(name -> localArtistUriFor(idCreator.createSha1("ARTIST:" + name)));
final URI recordUri = localRecordUriFor(idCreator.createSha1("RECORD:" + recordTitle));
final Optional<URI> newArtistUri = seenArtistUris.putIfAbsentAndGetNewKey(artistUri, true);
final Optional<URI> newRecordUri = seenRecordUris.putIfAbsentAndGetNewKey(recordUri, true);
statementManager.requestAddStatements()
.withOptional(trackUri, DC.TITLE, literalFor(title))
.withOptional(trackUri, RDFS.LABEL, literalFor(title))
.withOptional(trackUri, FOAF.MAKER, artistUri)
.with( recordUri, MO.P_TRACK, trackUri)
.withOptional(newArtistUri, RDF.TYPE, MO.C_MUSIC_ARTIST)
.withOptional(newArtistUri, RDFS.LABEL, literalFor(artistName))
.withOptional(newArtistUri, FOAF.NAME, literalFor(artistName))
.withOptional(newRecordUri, RDF.TYPE, MO.C_RECORD)
.withOptional(newRecordUri, RDFS.LABEL, literalFor(recordTitle))
.withOptional(newRecordUri, DC.TITLE, literalFor(recordTitle))
.withOptional(newRecordUri, MO.P_MEDIA_TYPE, MO.C_CD)
.withOptional(newRecordUri, FOAF.MAKER, artistUri)
.publish();
}
The last picky remark is about measuring test coverage. Tools such as Emma or JaCoCo can detect which lines of
code are covered by tests and which aren't, and produce a report consisting in the code listing with red and green
markers. In the original code we have branches and we could easily detect a red conditional body in case the test
didn't exercise it. With the refactored code we have lost this opportunity, because the branches have been masked
inside Optional and its treatment inside literalFor() and withOptional().
I don't think any new coverage tool can be smart enough to produce a coverage report whose readability is
comparable to the original one. In the end, I suppose it's the usual point that there's not such a thing as a free
lunch.