Code Smells: Deeply Nested Code

Or: I wants all your data, give it to me… my precious….

This is part of a series investigating code that looks suspicious (“code smells”), and exploring possible alternatives.

Continuing the series of code smells and what do about them, in this post I examine some fairly innocent looking code that defies the obvious refactoring.  Although the code example itself is fairly trivial, it’s actually a symptom of a problem found again and again in this particular project: deep nesting of code. This can be for loops, if statements, even lambda expressions or inner classes, or combinations of all of the above.

The Smell: Deeply Nested Code

The code I stumbled over was a double for loop with an inner if statement.

What’s wrong with this code?  It’s OK to read, if we forgive the single-character variable names, and are used to working in a procedural way – we’re looking at all the fields, and then for each field we look at all the names, and if a name matches the name we’re looking for, we return that field.  Simple.

Solution 1: Java 8 to the rescue

The lovely arrow shape of the code here is the bit that looks suspicious.  I’ve shown in previous blogs and talks that nested for/if statements can often be replaced with Java 8 Streams, and this frequently gives better readability.  I was wondering why IntelliJ IDEA doesn’t suggest this particular code can be collapsed into a Stream operation, seems like maybe flatMap/findFirst could be the answer. But it’s not that simple. I tried crafting the correct expression without automagic help from the IDE, and came up with:

Well, that’s no good. I want to return the mappedField that contains the name, not an Optional containing the name that I found. And I can’t get to mappedField later in the Stream. The best I can do is:

Urgh. Nasty.

Solution 2: Better Encapsulation

What’s the real problem here?  It seems like the original code is reaching deep inside another object, stealing all its data, riffling through it and then only caring about the top level object.  Removing the loop syntax, what you have is effectively:

This what the Law of Demeter warns us against, therefore suggests to me we consider a tell-don’t-ask approach.  Wouldn’t it be prettier to ask mappedField if one of its names matched the name we wanted?

The other for loop that loops over the names is moved inside the MappedField class, so only this class needs to understand the internals of how these names are stored.  Let’s step through one way of performing this refactoring.  There’s more than one approach of course, this is just one example.

Firstly, extract the inner loop to a separate method.  In this case, let’s call it hasName:

Extract Method hasName

Extract Method hasName

The extracted method doesn’t compile, but that’s OK because I wanted to change the method signature anyway – I want the method to return true if the name matches one of the MappedField‘s names, false otherwise.

Change method signature to return a boolean

Change method signature to return a boolean

Now I want to actually use this return value in the original method

Use the boolean value returned

Use the boolean value returned

The code compiles now and behaves the same way as the original.  Now I just need to move the hasName method into the MappedField class.

Move method into MappedClass

Move method into MappedClass

So now the MappedField has the hasName method:

…and the class containing the getMappedField method (MappedClass) no longer needs to know the internals of how the MappedField stores its names, or even that it can have more than one name.

Optional extra step: Java 8

If you so wish, both of these methods can now use Java 8 syntax. In the case of hasName, this is more descriptive too, it states we only care if any of the names matches the given one.

Java 8 Streams version of hasName

The hasName method can be converted to use Java 8 Streams

We end up with:

Meanwhile, getMappedField can also benefit from Java 8:

Java 8 version of getMappedField

The getMappedField method can be converted to use Java 8 Streams too

The final code for getMappedField looks more like I wanted the first time I saw it:

Final Step, Unrelated To This Code Smell

I stumbled over the original code when I was looking for methods that suit an Optional return type. This seems like a good candidate, it returns null in the case that the storedName doesn’t match any MappedField. Now we’re using the Java 8 syntax, not only is it clearer that we can use an Optional, but easier:

All we have to do is now deal with the returned Optional in the places that call this method, but that’s the subject for another blog post…

In Summary

In languages like Java it’s very normal to see multiple nested for loops and if statements dotted around the place, particularly in pre-Java-8 code. This sort of code is perfectly acceptable for manipulating low level data structures (arrays, collections etc), but should really be treated as a smell if you see it in your domain code, particularly if the nesting gets very deep.  In our example, although MappedClass needs to know about its MappedFields, it shouldn’t need to poke around the internals of MappedField to answer questions, it should be asking MappedField questions of its own.

Symptoms

  • Deeply nested code, usually loops and/or conditionals
  • Chained method calls that are all getters (or other non-builder, non-DSL method chains).

Possible solutions

  • Consider tell don’t ask. Where the code previously grabbed internal data and did checks against it, move those checks into the object containing the data and create a method with a useful name to help other objects answer their questions.
  • Examine your encapsulation. Does the data really need to leak out into other classes? Should data and/or responsibilities be relocated so that operations on the data are moved closer to the data itself?
  • Collection Encapsulation. Sometimes this pattern is a sign that you’re using a data structure as a domain object.  Consider encapsulating this data structure (Collection, array, etc) inside a domain object of its own and adding helper methods to let other objects ask it questions rather than poke around its internals.
This entry was posted in Tips & Tricks and tagged , . Bookmark the permalink.

5 Responses to Code Smells: Deeply Nested Code

  1. barmic says:

    I try to refactor the initial code before the reading and I write it :
    public Optional getMappedField(final String storedName) {
    return persistenceFields.stream()
    .filter(mappedField -> mappedField.getLoadNames().contains(storedName))
    .findFirst();
    }

    Why use Stream.anyMatch() instead Collection.contains() ?
    To improve the reading I can write it :

    public Optional getMappedField(final String storedName) {
    Predicate hasName = mappedField -> mappedField.getLoadNames().contains(storedName);
    return persistenceFields.stream()
    .filter(hasName)
    .findFirst();
    }

    But in a lot of case (a lot call to this function, a lot of data, not lot of update of persistenceFields, immutable MappedField) I think that use an index will lot improve the performance :

    public Optional getMappedField(final String storedName) {
    return Optional.ofNullable(nameIndex.get(storedName));
    }

    Thank you for this paper !

    • Trisha Gee says:

      I generally don’t like to use contains() because it relies on equals() having been implemented in a way that’s useful. Granted, in this case it’s fine because the collection is of Strings. Also, theoretically using Streams allows me to parallelise the operation if I need to (which I don’t just yet).

      My next blog post is going to address alternatives to this code – Java 8 Streams can sometimes simply be used to cover an underlying problem with the choice of data structure, which is what I think you’re saying with the last example.

  2. Witek says:

    You can also switch to Scala :)
    Then a method from an example becomes:

    def getMappedField(storedName: String): Option[MappedField] =
    persistenceFields.find(_.contains(storedName))

    • Witek says:

      Excuse me – a given example is not so simple.
      Actually scala code for it would be:

      def getMappedField(storedName: String): Option[MappedField] =
      persistenceFields.flatMap(_.find(_ == storedName)).headOption

      or if you want it to run fast on a big amount of data:

      def getMappedField(storedName: String): Option[MappedField] =
      persistenceFields
      .withFilter(_.exists(_ == storedName))
      .flatMap(_.find(_ == storedName))
      .headOption

  3. Witek says:

    Speakin with myself again :)
    Actually this code is meant to return whole MappedField – this makes this code much easier.
    Sorry for a mess in comments but I finally get it in a 3rd iteration:
    def getMappedField(storedName: String): Option[MappedField] =
    persistenceFields.find(_.contains(storedName))

    And this one is not only doing the actual stuff desired by the author of this article but is also easiest read.

Leave a Reply

Your email address will not be published. Required fields are marked *