- \n
- Code Smells: Null<\/a><\/li>\n
- Code Smells: Deeply Nested Code<\/strong><\/li>\n
- Code Smells: Iteration<\/a><\/li>\n
- Code Smells: Mutation<\/a><\/li>\n
- Code Smells:\u00a0Multi-Responsibility Methods<\/a><\/li>\n
- Code Smells: If Statements<\/a><\/li>\n
- Code Smells: Too Many Problems<\/a><\/li>\n<\/ul>\n
Continuing the series of code smells<\/a> and what do about them, in this post I examine some fairly innocent looking code that defies the obvious refactoring.\u00a0 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.<\/p>\n
<\/p>\n
The Smell: Deeply Nested Code<\/strong><\/p>\n
The code I stumbled over was a double for loop with an inner if statement.<\/p>\n
\u00a0\u00a0\u00a0 public MappedField getMappedField(final String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (final MappedField mf : persistenceFields) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (final String n : mf.getLoadNames()) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if (storedName.equals(n)) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return mf;\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return null;\r\n\u00a0\u00a0\u00a0 }<\/pre>\nWhat’s wrong with this code?\u00a0 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.\u00a0 Simple.<\/p>\n
Solution 1: Java 8 to the rescue<\/strong><\/p>\n
The lovely arrow\u00a0shape of the code<\/a> here is the bit that looks suspicious. \u00a0I’ve shown in previous blogs<\/a> and talks<\/a> that nested for\/if statements can often be replaced with\u00a0Java 8 Streams, and this frequently gives better readability.\u00a0 I was wondering why IntelliJ IDEA doesn’t suggest this particular code can be collapsed into a Stream operation, seems like maybe\u00a0flatMap<\/a>\/findFirst<\/a>\u00a0could 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:<\/p>\n
Optional<String> found = persistenceFields.stream()\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0.flatMap(mappedField -> mappedField.getLoadNames().stream())\r\n \u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .filter(storedName::equals)\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0.findFirst();<\/pre>\n
Well, that’s no good. I want to return the
mappedField<\/code> that contains the name, not anOptional<\/code> containing the name that I found. And I can’t get tomappedField<\/code> later in the Stream. The best I can do is:<\/p>\npersistenceFields.stream()\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0.filter(mappedField -> {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 \u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (String name : mappedField.getLoadNames()) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 \u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0if (storedName.equals(name)) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 \u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0return true;\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 \u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0}\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return false;\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0})\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0.findFirst()<\/pre>\nUrgh. Nasty.<\/p>\n
Solution 2: Better Encapsulation<\/strong><\/p>\n
What\u2019s the real problem here?\u00a0 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.\u00a0 Removing the loop syntax, what you have is effectively:<\/p>\n
if (getPersistenceFields().get(0).getLoadNames().get(0).equals(storedName))\r\n\u00a0\u00a0\u00a0 return true;<\/pre>\n
This what the Law of Demeter<\/a> warns us against, therefore suggests to me we consider a tell-don’t-ask approach<\/a>.\u00a0 Wouldn’t it be prettier to ask
mappedField<\/code> if one of its names matched the name we wanted?<\/p>\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (final MappedField mf : persistenceFields) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if (mf.hasName(storedName)) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return mf;\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }<\/pre>\nThe other for loop that loops over the names is moved inside the
MappedField<\/code> class, so only this class needs to understand the internals of how these names are stored.\u00a0 Let\u2019s step through one way of performing this refactoring.\u00a0 There’s more than one approach of course, this is just one example.<\/p>\nFirstly, extract the inner loop to a separate method.\u00a0 In this case, let’s call it
hasName<\/code>:<\/p>\n![\"Extract](\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-01ExtractMethod-2.png\")
<\/a><\/p>\nExtract Method hasName<\/p>\n<\/div>\n
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<\/code>‘s names, false otherwise.<\/p>\n![\"Change](\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-2ChangeMethodSignature-2.gif\")
<\/a><\/p>\nChange method signature to return a boolean<\/p>\n<\/div>\n
Now I want to actually use this return value in the original method<\/p>\n
![\"Use](\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-3UseBooleanValue-2.gif\")
<\/a><\/p>\nUse the boolean value returned<\/p>\n<\/div>\n
The code compiles now and behaves the same way as the original.\u00a0 Now I just need to move the
hasName<\/code> method into theMappedField<\/code> class.<\/p>\n![\"Move](\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-4MoveMethod-2.gif\")
<\/a><\/p>\nMove method into MappedClass<\/p>\n<\/div>\n
So now the
MappedField<\/code> has thehasName<\/code> method:<\/p>\n\u00a0\u00a0\u00a0 public boolean hasName(String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (final String name : getLoadNames()) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if (storedName.equals(name)) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return true;\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return false;\r\n\u00a0\u00a0\u00a0 }<\/pre>\n\u2026and the class containing the
getMappedField<\/code> method (MappedClass<\/code>) no longer needs to know the internals of how theMappedField<\/code> stores its names, or even that it can have more than one name.<\/p>\nOptional extra step: Java 8<\/strong><\/p>\n
If you so wish, both of these methods can now use Java 8 syntax. In the case of
hasName<\/code>, this is more descriptive too, it states we only care if any of the names matches the given one.<\/p>\n![\"Java](\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-5Java8NameMatches.png\")
<\/a><\/p>\nThe\u00a0
hasName<\/code> method can be converted to use Java 8 Streams<\/p>\n<\/div>\nWe end up with:<\/p>\n
\u00a0\u00a0\u00a0 public boolean hasName(String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return getLoadNames().stream()\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .anyMatch(storedName::equals);\r\n\r\n\u00a0\u00a0\u00a0 }<\/pre>\nMeanwhile,
getMappedField<\/code> can also benefit from Java 8:<\/p>\n![\"Java](\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-6Java8GetMappedField.png\")
<\/a><\/p>\nThe\u00a0getMappedField method can be converted to use Java 8 Streams too<\/p>\n<\/div>\n
The final code for
getMappedField<\/code> looks more like I wanted the first time I saw it:<\/p>\n\u00a0\u00a0\u00a0 public MappedField getMappedField(final String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return persistenceFields.stream()\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .filter(mf -> mf.hasName(storedName))\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .findFirst()\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .orElse(null);\r\n\u00a0\u00a0\u00a0 }<\/pre>\nFinal Step, Unrelated To This Code Smell<\/strong><\/p>\n
I stumbled over the original code when I was looking for methods that suit an
Optional<\/code> return type. This seems like a good candidate, it returns null in the case that the storedName doesn’t match anyMappedField<\/code>. Now we’re using the Java 8 syntax, not only is it clearer that we can use anOptional<\/code>, but easier:<\/p>\n\u00a0\u00a0\u00a0 public Optional<MappedField> getMappedField(final String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return persistenceFields.stream()\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .filter(mf -> mf.hasName(storedName))\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 .findFirst();\r\n\u00a0\u00a0\u00a0 }<\/pre>\nAll we have to do is now deal with the returned
Optional<\/code> in the places that call this method, but that’s the subject for another blog post\u2026<\/p>\nIn Summary<\/strong><\/p>\n
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.\u00a0 In our example, although
MappedClass<\/code> needs to know about itsMappedField<\/code>s, it shouldn’t need to poke around the internals ofMappedField<\/code> to answer questions, it should be askingMappedField<\/code> questions of its own.<\/p>\nSymptoms<\/p>\n
- \n
- Deeply nested code, usually loops and\/or conditionals<\/li>\n
- Chained method calls that are all getters (or other non-builder, non-DSL method chains).<\/li>\n<\/ul>\n
Possible solutions<\/p>\n
- \n
- 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.<\/li>\n
- 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?<\/li>\n
- Collection Encapsulation<\/a>. Sometimes this pattern is a sign that you’re using a data structure as a domain object. \u00a0Consider\u00a0encapsulating 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.<\/li>\n<\/ul>\n","protected":false},"author":360,"featured_media":0,"comment_status":"open","ping_status":"open","template":"","categories":[601],"tags":[3278,2993],"cross-post-tag":[],"acf":[],"_links":{"self":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea\/25929"}],"collection":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea"}],"about":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/types\/idea"}],"author":[{"embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/users\/360"}],"replies":[{"embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/comments?post=25929"}],"version-history":[{"count":0,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea\/25929\/revisions"}],"wp:attachment":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/media?parent=25929"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/categories?post=25929"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/tags?post=25929"},{"taxonomy":"cross-post-tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/cross-post-tag?post=25929"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}
- Code Smells: Deeply Nested Code<\/strong><\/li>\n