Code Smells: Deeply Nested Code

Posted on by

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.



public MappedField getMappedField(final String storedName) { for (final MappedField mf : persistenceFields) { for (final String n : mf.getLoadNames()) { if (storedName.equals(n)) { return mf; } } } return null; }

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:



Optional<String> found = persistenceFields.stream() .flatMap(mappedField -> mappedField.getLoadNames().stream()) .filter(storedName::equals) .findFirst();

mappedField

Optional

mappedField

persistenceFields.stream() .filter(mappedField -> { for (String name : mappedField.getLoadNames()) { if (storedName.equals(name)) { return true; } } return false; }) .findFirst()

Well, that’s no good. I want to return thethat contains the name, not ancontaining the name that I found. And I can’t get tolater 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:



if (getPersistenceFields().get(0).getLoadNames().get(0).equals(storedName)) return true;

mappedField

for (final MappedField mf : persistenceFields) { if (mf.hasName(storedName)) { return mf; } }

MappedField

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 askif one of its names matched the name we wanted?The other for loop that loops over the names is moved inside theclass, 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 :

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.

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

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.

So now the MappedField has the hasName method:



public boolean hasName(String storedName) { for (final String name : getLoadNames()) { if (storedName.equals(name)) { return true; } } return false; }

getMappedField

MappedClass

MappedField

…and the class containing themethod () no longer needs to know the internals of how thestores 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.

We end up with:



public boolean hasName(String storedName) { return getLoadNames().stream() .anyMatch(storedName::equals); }

getMappedField

Meanwhile,can also benefit from Java 8:

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



public MappedField getMappedField(final String storedName) { return persistenceFields.stream() .filter(mf -> mf.hasName(storedName)) .findFirst() .orElse(null); }

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:



public Optional<MappedField> getMappedField(final String storedName) { return persistenceFields.stream() .filter(mf -> mf.hasName(storedName)) .findFirst(); }

Optional

All we have to do is now deal with the returnedin 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 MappedField s, 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