{"id":25941,"date":"2017-08-23T12:06:28","date_gmt":"2017-08-23T12:06:28","guid":{"rendered":"https:\/\/blog.jetbrains.com\/idea\/?p=15742"},"modified":"2017-09-20T10:56:38","modified_gmt":"2017-09-20T10:56:38","slug":"code-smells-iteration","status":"publish","type":"idea","link":"https:\/\/blog.jetbrains.com\/pt-br\/idea\/2017\/08\/code-smells-iteration","title":{"rendered":"Code Smells: Iteration"},"content":{"rendered":"<p><em>This is part of a series investigating\u00a0code that looks suspicious\u00a0(&#8220;code smells&#8221;),\u00a0and exploring\u00a0possible alternatives.<\/em><\/p>\n<ul>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-null\/\" rel=\"bookmark\">Code Smells: Null<\/a><\/li>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-deeply-nested-code\/\" rel=\"bookmark\">Code Smells: Deeply Nested Code<\/a><\/li>\n<li><strong>Code Smells: Iteration<\/strong><\/li>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-mutation\/\">Code Smells: Mutation<\/a><\/li>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/09\/code-smells-multi-responsibility-methods\/\">Code Smells:\u00a0Multi-Responsibility Methods<\/a><\/li>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/09\/code-smells-if-statements\/\">Code Smells: If Statements<\/a><\/li>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/09\/code-smells-too-many-problems\/\">Code Smells: Too Many Problems<\/a><\/li>\n<\/ul>\n<p><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-deeply-nested-code\/\">Last time<\/a> we looked at some suspicious nested code, and I suggested that the best way to deal with this was to move some of it into a different class to stop the original class having to understand the internals of how the data was stored.<\/p>\n<p><strong>The Smell: Iteration<\/strong><\/p>\n<p>But that&#8217;s not the end of the story.\u00a0 In this article I want to explore a different problem that code iteration might be pointing to.\u00a0 The nested iteration in the last example suggested the logic was in the wrong place.\u00a0 The existence of iteration at all in our new <code>hasName<\/code> method (whether implemented as a for loop or using the Streams API) suggests another problem: this might not be the correct data structure to store the names in.<\/p>\n<p>This may even be hidden (or at least less obvious) when using the Streams API because this gives us a readable way to query our collection.\u00a0 But it doesn&#8217;t necessarily give us a <em>fast<\/em> way to query our collection.<\/p>\n<p><!--more--><\/p>\n<p><strong>Example 1: Set instead of List<\/strong><\/p>\n<p>Let&#8217;s take a closer look at the code in question. In the\u00a0<a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-deeply-nested-code\/\">Deeply Nested Code article<\/a> we created <code>hasName()<\/code> in <code>MappedField<\/code><\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"MappedField.hasName()\">\u00a0\u00a0\u00a0 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\u00a0\u00a0\u00a0 }<\/pre>\n<p>I&#8217;m going to expand it back out to a for loop so it&#8217;s more obvious what&#8217;s happening<\/p>\n<div id=\"attachment_15744\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-01-turn-into-loop.gif\" rel=\"attachment wp-att-15744\"><img aria-describedby=\"caption-attachment-15744\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15744\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-01-turn-into-loop.gif\" alt=\"Turn Streams API call into a for loop\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15744\" class=\"wp-caption-text\">Turn Streams API call into a for loop<\/p>\n<\/div>\n<p>We end up with<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"Using a for loop instead of Java 8\">\u00a0 \u00a0 boolean hasName(String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (String s : getLoadNames()) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if (storedName.equals(s)) {\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 }\r\n<\/pre>\n<p>The <code>getLoadNames<\/code> method returns a List of Strings, which we iterate over in order to see if a particular String value is in there.\u00a0 If we look at the other places <code>getLoadNames<\/code> is used, the usage is pretty similar &#8211; we iterate over the list to find something we want.<\/p>\n<div id=\"attachment_15745\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-02-find-usages.gif\" rel=\"attachment wp-att-15745\"><img aria-describedby=\"caption-attachment-15745\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15745\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-02-find-usages.gif\" alt=\"Find usages of getLoadNames()\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15745\" class=\"wp-caption-text\">Find usages of getLoadNames()<\/p>\n<\/div>\n<p>This seems pretty wasteful &#8211; why iterate over the same list again and again, when you can store it in a data structure which lets you ask immediately if it contains what you want? Given that in this particular case, the names are either unique, or duplicates are effectively ignored, it seems that <code>Set<\/code> might be the data structure we want instead.<\/p>\n<p>The <code>getLoadNames<\/code> method is not a simple getter, as you might expect from the name (but misleading naming is not the topic of this blog post, nor is caching the results of an operation that only needs to be done once, so let&#8217;s gloss over those smells for now).\u00a0 It processes some data in order to calculate the list of names. The implementation details aren&#8217;t important for the purpose of this article, we&#8217;re just going to do the simplest thing that works and change the method to return a Set.<\/p>\n<div id=\"attachment_15746\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-03-return-set.gif\" rel=\"attachment wp-att-15746\"><img aria-describedby=\"caption-attachment-15746\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15746\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-03-return-set.gif\" alt=\"Return a Set from getLoadNames()\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15746\" class=\"wp-caption-text\">Return a Set from getLoadNames()<\/p>\n<\/div>\n<p>Because Set is also a Collection (like the original return type List), all of the code that calls this method still passes &#8211; the three callers only used the method in a for loop. Going back to our original method <code>hasName<\/code>, we can simplify it to:<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"\">\u00a0\u00a0\u00a0 boolean hasName(String storedName) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return getLoadNames().contains(storedName);\r\n\u00a0\u00a0\u00a0 }<\/pre>\n<p>That&#8217;s it.\u00a0 No more looping, just a simple check.<\/p>\n<p>We should look at the other callers to see if they can be simplified as well, but if they need to iterate over the Set (for any reason) we can leave them as they are.<\/p>\n<p>Running all the tests for the project shows everything still works as expected so we can commit these changes.<\/p>\n<p><strong>Example 2: Map instead of List<\/strong><\/p>\n<p>Here&#8217;s a very similar example, with a slightly different solution:<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"MappedClass.getMappedFieldByJavaField()\">\u00a0\u00a0\u00a0 public MappedField getMappedFieldByJavaField(final String name) {\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 if (mf.getJavaFieldName().equals(name)) {\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 }\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return null;\r\n\u00a0\u00a0\u00a0 }<\/pre>\n<p>In this method we&#8217;re iterating over all the fields, and looking for one with the same <code>javaFieldName<\/code> as some given value.\u00a0 This sounds like something which would be better handled by a different data structure. In this case, a Map of <code>String<\/code> name to <code>MappedField<\/code> would let us immediately find the right field with this name.<\/p>\n<p>The <code>persistenceFields<\/code> field is used in a number of places in this class, so it&#8217;s best to take a safe approach to migrating its data structure. \u00a0I&#8217;m going to replace it with a new field, <code>fields<\/code>, which is a Map of <code>String<\/code> to <code>MappedField<\/code>.<\/p>\n<div id=\"attachment_15750\" style=\"width: 1110px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-04-new-fields-field-2.png\" rel=\"attachment wp-att-15750\"><img aria-describedby=\"caption-attachment-15750\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15750\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-04-new-fields-field-2.png\" alt=\"Create a new Map field\" width=\"1100\" height=\"154\" \/><\/a><\/p>\n<p id=\"caption-attachment-15750\" class=\"wp-caption-text\">Create a new Map field<\/p>\n<\/div>\n<p>I use highlight usages to find everywhere in this class that <code>persistenceFields<\/code> was used. The first step is to initialise the Map in the same place the <code>List<\/code> was initialised<\/p>\n<div id=\"attachment_15754\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-05-initialise-map-3.gif\" rel=\"attachment wp-att-15754\"><img aria-describedby=\"caption-attachment-15754\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15754\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-05-initialise-map-3.gif\" alt=\"Initialise the new Map\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15754\" class=\"wp-caption-text\">Initialise the new Map<\/p>\n<\/div>\n<p>Then I can find all the places that queried <code>persistenceFields<\/code>, and the simplest way to migrate them to use the new field is to use the <code>values()<\/code> Collection from the Map, so all the existing code works the same way it used to<\/p>\n<div id=\"attachment_15748\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-06-use-values.gif\" rel=\"attachment wp-att-15748\"><img aria-describedby=\"caption-attachment-15748\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15748\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-06-use-values.gif\" alt=\"Use Map.values() instead of original list\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15748\" class=\"wp-caption-text\">Use Map.values() instead of original list<\/p>\n<\/div>\n<p>Most of these changes only impact this class and not other code.\u00a0 The exception is the getter, which we have to change to return a Collection rather than a List. A quick look at the usages of this method show that the List is only used for iteration, or other cases where a Collection will work just as well.\u00a0 So we change this to return the Collection<\/p>\n<div id=\"attachment_15749\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-07-return-collection.gif\" rel=\"attachment wp-att-15749\"><img aria-describedby=\"caption-attachment-15749\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15749\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-07-return-collection.gif\" alt=\"Change the return type of the getter\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15749\" class=\"wp-caption-text\">Change the return type of the getter<\/p>\n<\/div>\n<p>When we&#8217;ve replaced the usages of the original List with the new Map, we can safely remove the old List and rename the Map (if we like) to the name of the original field.<\/p>\n<div id=\"attachment_15743\" style=\"width: 1290px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-08-remove-list.gif\" rel=\"attachment wp-att-15743\"><img aria-describedby=\"caption-attachment-15743\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15743\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-08-remove-list.gif\" alt=\"Remove original list\" width=\"1280\" height=\"720\" \/><\/a><\/p>\n<p id=\"caption-attachment-15743\" class=\"wp-caption-text\">Remove original list<\/p>\n<\/div>\n<p>Rebuilding the project and re-running the tests shows everything still compiles and runs as expected.<\/p>\n<p>Now we can go back to the original method with the smell, and make the use of the map.\u00a0 The updated method is:<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"Updated getMappedFieldByJavaField\">\u00a0\u00a0\u00a0 public MappedField getMappedFieldByJavaField(final String name) {\r\n\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 return persistenceFields.get(name);\r\n\u00a0\u00a0\u00a0 }<\/pre>\n<p>Much simpler!<\/p>\n<p>If there are other methods that were iterating over the list to find the name, we can make the same change (or simply delegate to <code>getMappedFieldByJavaField<\/code>).\u00a0 All callers that were iterating for any other reason still work exactly the same as before, just iterating over the Map&#8217;s <code>values()<\/code>. We may want to look at these other methods later to see if there&#8217;s some way to simplify them.\u00a0 Remember, it&#8217;s OK to have more than one data structure to store the same information, if the purpose is to provide fast access to the data &#8211; this is particularly useful if reading happens more frequently than updating.\u00a0 In our case, we might want to store our <code>MappedField<\/code>s in several Maps keyed by different values.<\/p>\n<p><strong>In Summary<\/strong><\/p>\n<p>If you see iteration code (for loops or Streams API calls) you should question whether this is really needed.\u00a0 If the data structure is always being iterated over in order to find some value, you may be better off using a Set or a Map. This will give you not only simpler, cleaner code, but usually better performance as well.<\/p>\n<p>Remember when choosing a data structure that there are two main access patterns to consider &#8211; adding\/updating data, and reading the data.\u00a0 How frequently you read vs write and what sorts of update operations you do will determine which is the most efficient data structure for you.<\/p>\n<p>Symptoms:<\/p>\n<ul>\n<li>For loops iterating over a collection looking for some value<\/li>\n<li>Streams API calls with <code>findFirst<\/code>\/<code>findAny<\/code>\/<code>anyMatch<\/code>.<\/li>\n<\/ul>\n<p>Possible Solutions:<\/p>\n<ul>\n<li>If the code is checking if a value exists in a List, consider using a Set instead, if the values are unique or duplicates are ignored.<\/li>\n<li>If the code is always looking for an Object with a particular property, a Map of the Objects keyed by that property will be much more efficient.<\/li>\n<li>The Collections API gives you a lot of useful methods too, so even if you can&#8217;t change the data structure directly, you should check if there&#8217;s a Collection method that does what you want.\u00a0 Our first example could have continued being a List, if duplicates were expected, but we could have used <code>List.contains()<\/code> instead of writing out own iteration code.\u00a0 This is more concise, and the Collections implementation is usually the most efficient version for the data structure.<\/li>\n<li>If, for some reason, you really do need to write your own iteration code, it may be best to <a href=\"https:\/\/refactoring.com\/catalog\/encapsulateCollection.html\" target=\"_blank\" rel=\"noopener\">encapsulate the implementation details<\/a> of how data is written\/read, and provide nice helpfully named methods for your domain objects to use this. This way at least the iteration code is kept out of your business logic.<\/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\/pt-br\/wp-json\/wp\/v2\/idea\/25941"}],"collection":[{"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/idea"}],"about":[{"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/types\/idea"}],"author":[{"embeddable":true,"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/users\/360"}],"replies":[{"embeddable":true,"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/comments?post=25941"}],"version-history":[{"count":0,"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/idea\/25941\/revisions"}],"wp:attachment":[{"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/media?parent=25941"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/categories?post=25941"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/tags?post=25941"},{"taxonomy":"cross-post-tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/pt-br\/wp-json\/wp\/v2\/cross-post-tag?post=25941"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}