IntelliJ IDEA
IntelliJ IDEA – the Leading Java and Kotlin IDE, by JetBrains
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.
- Code Smells: Null
- Code Smells: Deeply Nested Code
- Code Smells: Iteration
- Code Smells: Mutation
- Code Smells: Multi-Responsibility Methods
- Code Smells: If Statements
- Code Smells: Too Many Problems
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();
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:
persistenceFields.stream() .filter(mappedField -> { for (String name : mappedField.getLoadNames()) { if (storedName.equals(name)) { return true; } } return false; }) .findFirst()
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;
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?
for (final MappedField mf : persistenceFields) { if (mf.hasName(storedName)) { return mf; } }
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
:
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; }
…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.
We end up with:
public boolean hasName(String storedName) { return getLoadNames().stream() .anyMatch(storedName::equals); }
Meanwhile, getMappedField
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); }
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:
public Optional<MappedField> getMappedField(final String storedName) { return persistenceFields.stream() .filter(mf -> mf.hasName(storedName)) .findFirst(); }
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 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
- 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.