The Pile Up

Fluent programming is great. Love it. However, it’s easy to misuse it to produce a collision of coding moves where there’s quite a complex detailed implementation all bundled into a single line.

Frequently this can be seen in Java streaming operations where the lambdas have been expressed long-form inline and it becomes hard to tell what’s going on.

In this article, I’m going to show you an example of such a pile up and try to refactor it to make sense of what’s going on. There’s one person in the world who might take offence at seeing this treatment of this exact code. To that person I kindly suggest that a small amount of consideration to future readers rather than to the process of writing the code would have gone a long way.

Imagine you were faced with this (scroll right…):

    public Optional<String> getLatestSchema(String subject) {
        return schemas.containsKey(subject) ?
                Optional.of(new PropertyLoader().getProperties().getProperty(SCHEMA_LOCATION).concat("/").concat(schemas.get(subject).get(schemas.get(subject).keySet().stream().max(Version::compareTo).orElse(new Version(0,0,0)))))
                    : Optional.empty();
    }

Wowser! What an implementation! What does it do? Well, it gets the latest schema: that much we know from the javadoc-less function declaration.

The schemata are in a structure – Map<String, Map<Version, String>> schemas – where the primary key is the subject and the inner map maps a Version (a nice local POJO) to the filename of the Schema we need.

Let’s start taking the function apart.

The ternary expression is not helping readability in this case. Ternaries are good for short simple expressions. By the time we get to the : in this case, we’ve forgotten why we got there. The brain can only process a half a dozen things in one go. Let’s simplify by reversing the guard condition.

    public Optional<String> getLatestSchema(String subject) {
        if (!schemas.containsKey(subject)) {
            return Optional.empty();
        }
        return Optional.of(new PropertyLoader().getProperties().getProperty(SCHEMA_LOCATION).concat("/").concat(schemas.get(subject).get(schemas.get(subject).keySet().stream().max(Version::compareTo).orElse(new Version(0,0,0)))));
    }

Ok. So now we know that no subject is present means empty.

Now where? I think there’s probably something going on in the use of concat. It’s a very expensive way to build a string in my view… let’s look at how we can explain it and maybe make it less effortful to the machine and the reader.

It looks like there’s a property reader reading the SCHEMA_LOCATION and it looks like that’s being called every time this function is called. But the properties don’t vary at runtime. Let’s extract a constant.

private static final String BASE_PATH = 
    new PropertyLoader().getProperties()
        .getProperty(SCHEMA_LOCATION).concat("/");

We can now refactor that a little more.

private static final String BASE_PATH = 
    new PropertyLoader().getProperties()
        .getProperty(SCHEMA_LOCATION) +
        File.separator;

Let’s not define our own separator, and let’s use good old-fashioned + which the compiler will even do something magical with.

The code with the constant now looks like this (scroll right):

    public Optional<String> getLatestSchema(String subject) {
        if (!schemas.containsKey(subject)) {
            return Optional.empty();
        }
        return Optional.of(BASE_PATH.concat(schemas.get(subject).get(schemas.get(subject).keySet().stream().max(Version::compareTo).orElse(new Version(0,0,0)))));
    }

It seems like we can do an extract method on the inner get… the where?… exactly… Let’s even call the extracted method foo just so we can defer giving it a better name until we can see what’s going on.

    public Optional<String> getLatestSchema(String subject) {
        if (!schemas.containsKey(subject)) {
            return Optional.empty();
        }
        return Optional.of(
            BASE_PATH.concat(schemas.get(subject)
                .get(foo(subject))));
    }

    private Version foo(String subject) {
        return schemas.get(subject).keySet().stream()
            .max(Version::compareTo).orElse(new Version(0,0,0));
    }

Ok. So foo is somehow getting us a version and it’s using the keySet of the schemas.get(subject) to do it. I see two copies of schemas.get(subject) in this code and I also see more concat. Let’s refactor that out and rename the method.

So we wind up with:

    public Optional<String> getLatestSchema(String subject) {
        if (!schemas.containsKey(subject)) {
            return Optional.empty();
        }
        Map<Version, String> subjectVersions = 
            schemas.get(subject);
        return Optional.of(BASE_PATH +
            subjectVersions.get(getLatestVersion(subjectVersions)));
    }

    private Version getLatestVersion(Map<Version, String> subjectVersions) {
        return subjectVersions.keySet().stream()
            .max(Version::compareTo).orElse(new Version(0,0,0));
    }

Let’s look now at what getLatestVersion is doing. It’s returning the maximum version, or a made-up version if no version can be found… which is weird because that would cause the subjectVersions.get(...) in the other method to return null which would crash the Optional.of. We can refactor this to remove the bug by letting the subroutine return Optional instead.

    public Optional<String> getLatestSchema(String subject) {
        if (!schemas.containsKey(subject)) {
            return Optional.empty();
        }
        Map<Version, String> subjectVersions = 
            schemas.get(subject);
        Optional<Version> latestVersion = getLatestVersion(subjectVersions);
        if (!latestVersion.isPresent()) {
            return Optional.empty();
        }
        return Optional.of(BASE_PATH + subjectVersions.get(latestVersion.get()));
    }

    private Optional<Version> getLatestVersion(
            Map<Version, String> subjectVersions) {

        return subjectVersions.keySet().stream()
            .max(Version::compareTo);
    }

So far, I’ve put minimal effort into simplifying the main method, but I think the subroutine is about right. It neatly plucks the max from the subjectVersions map. Let’s go back to the main function.

The main function could use Optional.map

    public Optional<String> getLatestSchema(String subject) {
        if (!schemas.containsKey(subject)) {
            return Optional.empty();
        }
        Map<Version, String> subjectVersions = 
            schemas.get(subject);
        return getLatestVersion(subjectVersions)
                .map(subjectVersions::get)
                .map(version -> BASE_PATH + version);
    }

This is moving back toward fluent programming, but it’s somehow simpler. Notice how I used two map operations to represent pulling the filename, and then prefixing it with the base path.

But perhaps this whole thing can be reworked without the initial guard condition as a sort of Optional mapping stream thing.

    public Optional<String> getLatestSchema(String subject) {
        return Optional.ofNullable(schemas.get(subject))
                .flatMap(this::getLatestVersionForSubject);
    }
    
    public Optional<String> getLatestVersionForSubject(
            Map<Version, String> subjectVersions) {
        return getLatestVersion(subjectVersions)
                .map(subjectVersions::get)
                .map(version -> BASE_PATH + version);
    }

    private Optional<Version> getLatestVersion(
            Map<Version, String> subjectVersions) {
        return subjectVersions.keySet()
                .stream()
                .max(Version::compareTo);
    }

And there you have it. All written in a fluent form, with the use of subroutines to express more complex concepts. The parent method has to use Optional.ofNullable to deal with making empty from the lack of presence of the subject in the map, but that’s preferable to a long-handed guard condition. Similarly, there’s a use of flatMap to allow merging in the value from the Optional returned by the getLatestVersionForSubject method.

It’s my view that the above code is made of readable pieces, where the original was a ball of unreadable write only code with a potential bug in it.

That’s my opinion anyway. Opinions should be loosely held. Feel free to disagree.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s