{"id":25958,"date":"2017-08-29T10:24:19","date_gmt":"2017-08-29T10:24:19","guid":{"rendered":"https:\/\/blog.jetbrains.com\/idea\/?p=15773"},"modified":"2019-01-08T17:48:22","modified_gmt":"2019-01-08T17:48:22","slug":"code-smells-mutation","status":"publish","type":"idea","link":"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-mutation\/","title":{"rendered":"Code Smells: Mutation"},"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><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-iteration\/\">Code Smells: Iteration<\/a><\/li>\n<li><strong>Code Smells: Mutation<\/strong><\/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>In the article about <a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-deeply-nested-code\/\">deeply nested code<\/a>\u00a0I wanted to change <code>getMappedField<\/code> to return an <code>Optional<\/code> value. The change to the method was fairly trivial, the change to the code that calls that method&#8230; well, that turned out to be another matter altogether.<\/p>\n<p><!--more--><\/p>\n<p>One of the places this method is used is in <code>validateQuery<\/code>, a complex method that does rather a lot more than you might expect. When I\u00a0changed <code>getMappedField<\/code> to return an Optional, I ended up replacing null checks with a surprisingly large number of <code>isPresent<\/code> checks:<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-02-ispresent-checks.png\" rel=\"attachment wp-att-15786\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15786\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-02-ispresent-checks.png\" alt=\"Large number of isPresent checks\" width=\"1390\" height=\"1236\" \/><\/a><\/p>\n<p>That&#8217;s not all of them; this method is nearly a hundred lines long, and it doesn&#8217;t fit in a single screenshot (this will be a topic for another article). My first reaction was irritation that <code>Optional<\/code> not only didn&#8217;t solve my null check problem, but made my code uglier with the addition of these checks and the associated gets. But of course this was just a sign of other underlying problems.<\/p>\n<p><strong>The Smell: Mutable Values<\/strong><\/p>\n<p>This method is long enough to contain more than one smell. For this article, I want to focus on the first issue I noticed: the <code>mf<\/code> variable is exactly that &#8211; variable. It&#8217;s a mutable value, and is reassigned under some circumstances. We can see this if we make <code>mf<\/code> final.<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-03-field-reassigned.png\" rel=\"attachment wp-att-15787\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone wp-image-15787 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-03-field-reassigned.png\" alt=\"Value is resassigned\" width=\"1390\" height=\"403\" \/><\/a><\/p>\n<p>If the <code>getMappedField<\/code> method doesn&#8217;t return a value (and the <code>fieldIsArrayOperator<\/code> flag is false, whatever that means) we go off and try to assign <code>mf<\/code> to the value from <code>getMappedFieldByJavaField<\/code>. I assume readers don&#8217;t have a clear idea of the context of this specific code, but to be honest I&#8217;ve been working with this project on and off for years and I don&#8217;t really understand what it&#8217;s doing either. This is what being a developer is all about: it&#8217;s hard to write code, but it&#8217;s often harder to read it.<\/p>\n<p>Anyway.<\/p>\n<p>The point is this <code>mf<\/code>\u00a0value is being changed, and this makes it even harder to reason about.\u00a0 For example, when we check if it&#8217;s present later in the code, if the value is empty we don&#8217;t know if it&#8217;s empty because <code>getMappedField<\/code> returned no value or if it&#8217;s because <code>getMappedFieldByJavaField<\/code> returned no value. We just don&#8217;t know what it means if it is empty.<\/p>\n<p>We could try and address this specific problem by moving the logic that can reassign the value into its own method and giving that a useful name. Then we <em>should<\/em>\u00a0have an idea of what our variable is and why it may, or may not, be present. If we ask IntelliJ IDEA to extract this code into a method&#8230;<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-04-extract-method-fail.png\" rel=\"attachment wp-att-15774\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15774\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-04-extract-method-fail.png\" alt=\"Extract method fails\" width=\"1390\" height=\"392\" \/><\/a><\/p>\n<p>&#8230;it can&#8217;t be done, because of at least one other mutable value.<\/p>\n<p>We&#8217;re modifying the values inside a <code>pathElements<\/code> array. This doesn&#8217;t stop us from doing the refactoring, as we can pass this array in as a parameter and alter it in the method, but there is a problem with altering this value. More on this later.<\/p>\n<p>The modification that stops us from being able to extract the method is when we&#8217;re setting a flag <code>hasTranslations<\/code> to true under some circumstances. Turns out that this flag is an optimization \u2013 if it has been set to true, then we need to modify <em>another<\/em> value:<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-05-reassign-origProp.png\" rel=\"attachment wp-att-15775\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15775\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-05-reassign-origProp.png\" alt=\"Parameter origProp is changed\" width=\"1390\" height=\"273\" \/><\/a><\/p>\n<p>If we look more closely at this code, we might wonder about two things. First, why are we changing a value with the name <code>origProp<\/code>? It&#8217;s not going to be the original property after the change, is it? Second, why are we reusing and resetting a <code>StringBuilder<\/code>? Seems like an odd thing to do, rather than using a new String value, or setting a String value in some domain object.<\/p>\n<p>Turns out we&#8217;re changing a <code>StringBuilder<\/code>\u00a0because this code intentionally alters an input parameter value. This code not only makes heavy use of mutability to set the state within a long and complicated method, but it actually alters the values of the input parameters, presumably because Java doesn&#8217;t support multiple output values and this seemed like an OK compromise (note: it&#8217;s not).<\/p>\n<p>So to summarize the problems introduced here by mutating values:<\/p>\n<ol>\n<li>Mutating the value of <code>mf<\/code> makes the complex logic that follows very difficult to reason about.<\/li>\n<li>Setting a boolean flag to be used later in the method makes it very difficult to extract sections of the code into more readable small methods.<\/li>\n<li>Changing an input parameter adds extra complexity into this method and, most importantly, is potentially <em>extremely<\/em>\u00a0surprising to the caller of this method \u2013 we generally don&#8217;t expect our parameters to be changed when we pass them into a method.<\/li>\n<\/ol>\n<p>We have a long, complex, difficult to understand method that is extremely hard to refactor under these circumstances. And that&#8217;s just looking at the mutability smells in this code.<\/p>\n<p>Unlike previous articles which showed similar but unrelated examples, the refactoring suggestions in this article takes the form of a series of steps that need to be performed in order.\u00a0These steps may not be the first that occur to you (they weren&#8217;t the first ideas I tried), but they do try to address one small problem at a time and impact the code as little as possible.<\/p>\n<p><strong>Step 1: Stop mutating the array we&#8217;re iterating over<\/strong><\/p>\n<p>The first problem I&#8217;m going to tackle is the mutation of the <code>pathElements<\/code> array. By removing the code that alters <code>pathElements<\/code>, I hope to a) get rid of the rather nasty smell of altering an array\/collection I am iterating over and b) remove the need for the <code>hasTranslations<\/code> flag. Part b) should help us extract some of this enormous code into a simpler method which will limit the mutation of the <code>mf<\/code> variable to just one, easier-to-reason-about method.<\/p>\n<p>First I\u00a0introduce another object to store the new values (rather than changing the original object to contain the new values). I&#8217;ve chosen to use a <code>List&lt;String&gt;<\/code> for this, because I like working with collections and because Streams gives us a nice feature which I&#8217;ll use later in the method. I could just have easily have chosen an array of Strings; in this case it&#8217;s totally personal choice (you might be trading readability for performance, so this will depend upon your own code).<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-06-introduce-databasepath.png\" rel=\"attachment wp-att-15776\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15776\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-06-introduce-databasepath.png\" alt=\"Introduce databasePathElements\" width=\"1390\" height=\"63\" \/><\/a><\/p>\n<p>I create and initialize this collection in the same place the original <code>pathElements<\/code> array was created, and I&#8217;m going to initialize it with the same values.<\/p>\n<p>Next, where I changed the values in <code>pathElements<\/code>, I now want to change the values in <code>databasePathElements<\/code>. For now, I&#8217;m going to carry on doing both changes to try and reduce the chances of the code breaking.<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-07-set-new-values-on-databasepath.png\" rel=\"attachment wp-att-15777\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15777\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-07-set-new-values-on-databasepath.png\" alt=\"Set new values on databasePathElements\" width=\"1390\" height=\"152\" \/><\/a><\/p>\n<p>Finally, I&#8217;m going to use <code>databasePathElements<\/code> instead of <code>pathElements<\/code> to set the new values of the <code>StringBuilder<\/code> <code>origProp<\/code>.<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-08-use-databasepath-for-stringbuffer.png\" rel=\"attachment wp-att-15778\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15778\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-08-use-databasepath-for-stringbuffer.png\" alt=\"Use databasePathElements to set the StringBuffer\" width=\"1390\" height=\"153\" \/><\/a><\/p>\n<p>Note that I&#8217;ve used one of my favorite features from Java 8, <code>Collectors.joining<\/code>. This has replaced 5 lines of code with a single line \u2013 it&#8217;s really useful for producing CSV content or, in this case, turning a collection into a String separated by dots. The performance of this method may be different from the original code (plus we&#8217;re using a Collection instead of an array), so if nano-second performance matters to you, you should performance-test this. I expect for most applications the impact is negligible, and the new syntax reduces boilerplate.<\/p>\n<p>When I run all the tests, everything still passes. I go back and remove the code that alters <code>pathElement<\/code> (it was on line 76, here I&#8217;ve clicked on the blue change indicator in the gutter in IntelliJ IDEA to show the old code vs the new code).<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-10-delete-assignment.png\" rel=\"attachment wp-att-15780\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15780\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-10-delete-assignment.png\" alt=\"Delete line that changes array\" width=\"1390\" height=\"181\" \/><\/a><\/p>\n<p>Running the tests again shows this all works as before.<\/p>\n<p>This is a small change and I&#8217;ve limited it to only impact the internals of this method to minimize the chances of errors. It may seem a bit pointless but it is step one to reducing the complexity in this method.<\/p>\n<p><strong>Step 2: Remove hasTranslations flag<\/strong><\/p>\n<p>This flag is causing me all sorts of trouble when I try to extract a smaller method. One approach is to perform a check between the original path and our new path, to see if it has changed instead of making a note of when it has changed.<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-11-check-if-path-changed.png\" rel=\"attachment wp-att-15781\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15781\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-11-check-if-path-changed.png\" alt=\"Check if the path has changed\" width=\"1390\" height=\"186\" \/><\/a><\/p>\n<p>This is clearly a more expensive check (performance-wise) than a simple boolean check \u2013 not only do we do a String comparison, but we also have to build our new <code>databasePath<\/code> String regardless of whether we&#8217;re going to use it or not. I assume the original boolean was there to save us having to build the new String if we didn&#8217;t need it (there is no test or documentation so I&#8217;m guessing),\u00a0so this latest version of the code doesn&#8217;t seem to have any real value.<\/p>\n<p>Given that Morphia a) uses <code>java.lang.reflect<\/code> extensively and b) is designed to talk over a network to a database, nano-second CPU-instruction-level performance is not our biggest concern. Also, given there are no performance tests\/perf criteria for this part of the code, I&#8217;m going to make an executive decision and simply update the value with <code>databasePath<\/code> every time, regardless of whether it&#8217;s different or not. I expect the performance impact to be negligible, and the readability impact will be very positive.<\/p>\n<p>So I remove the check:<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-12-remove-check.png\" rel=\"attachment wp-att-15782\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15782\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-12-remove-check.png\" alt=\"Remove the check completely\" width=\"1390\" height=\"303\" \/><\/a><\/p>\n<p>&#8230;and now I can remove the flag:<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15791\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-13-remove-hasTranslations-prev.png\" alt=\"Remove the flag\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-13-remove-hasTranslations.gif\" \/><\/p>\n<p>Running all my tests again shows everything still works.<\/p>\n<p><strong>Step 3: Extract Method<\/strong><\/p>\n<p>Now I&#8217;ve removed this flag and introduced a new Collection for storing the updated database path, I should be able to extract a method that limits the scope of the mutation of <code>MappedField<\/code> <code>mf<\/code> to a small, easy-to-reason-about method.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15792\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-14-extract-method-prev.png\" alt=\"Extract method for getting the MappedField\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-14-extract-method.gif\" \/><\/p>\n<p>It&#8217;s not a particularly pretty method, as it takes a lot of parameters for just a few lines of code, but these can be simplified later. Our focus right now is limiting mutability and making the very long method easier to reason about.<\/p>\n<p><strong>Step 4: Introduce a new type<\/strong><\/p>\n<p>Next I want to tackle the smell of altering the value of one of the input parameters. If we needed to return more than one value, we either need to change the structure of the object that&#8217;s being returned, or return a new type that encapsulates all the returned values.<\/p>\n<p>In order to decide which approach to take, we need to think about the values we&#8217;re talking about: what are they, where do they live, what other data are they associated with? The two return values from this method are the <code>MappedField<\/code> that is the original return type, and a String value that represents this field&#8217;s path in the MongoDB database. I can&#8217;t add a new value to <code>MappedField<\/code> to store its database name for the simple reason that the returned <code>MappedField<\/code> is sometimes null, but the database path is always known (it&#8217;s part of the query so we always have it).<\/p>\n<p>The simplest solution to our problem, therefore, is to change this method to return a new type, <code>ValidatedField<\/code>, which wraps the original <code>MappedField<\/code> that was returned, and the String representing the database path. It&#8217;s not the prettiest solution at this stage, but given the current state of the method, a) anything to simplify the method is an improvement, and b) we may find ourselves using this for later refactoring, or we may find that this is a missing domain concept (we may rename it later when we discover what its true purpose is).<\/p>\n<p>The first part of this is to introduce a new type which wraps the <code>MappedField<\/code>. I&#8217;m going to use an inner class right now; I may refactor this later if\/when it becomes clear where it lives. I&#8217;m also going to use an <code>Optional<\/code> field, even though this is generally not recommended. It&#8217;ll make the refactoring a bit more straightforward, and I can always address this later.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15793\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-15-introduce-type-prev.png\" alt=\"Introduce new ValidatedField\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-15-introduce-type.gif\" \/><\/p>\n<p>I can remove <code>retVal<\/code> because it&#8217;s not being used anymore.<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-16-remove-retval.png\" rel=\"attachment wp-att-15783\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15783\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-16-remove-retval.png\" alt=\"Remove retVal\" width=\"1390\" height=\"209\" \/><\/a><\/p>\n<p>I&#8217;ve changed the method to return this new <code>ValidatedField<\/code> type, so I&#8217;m going to have to fix up any compilation errors from this \u2013 anywhere that calls the original method is expecting a <code>MappedField<\/code> not a <code>ValidatedField<\/code>.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15794\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-17-change-callers-prev.png\" alt=\"Change callers to use new type\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-17-change-callers.gif\" \/><\/p>\n<p>For now I&#8217;ve done the simplest thing that works and getting the <code>MappedField<\/code> from the new <code>ValidatedField<\/code>. Rerunning all the tests shows this works as expected.<\/p>\n<p><strong>Step 5: Return the database path value instead of mutating a parameter<\/strong><\/p>\n<p>The next step is to add the database path to this <code>ValidatedField<\/code> object, so that we can stop the travesty that is the mutation of the <code>StringBuilder<\/code> parameter.<\/p>\n<p>Firstly, we add a database path field to <code>ValidatedField<\/code>.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15795\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-18-add-database-path-to-validated-field-prev.png\" alt=\"Add database path to ValidatedField\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-18-add-database-path-to-validated-field.gif\" \/><\/p>\n<p>I&#8217;m not going to remove the old code just yet, but I&#8217;m going to move over to using the new value first. I locate all the callers of this method using <a href=\"https:\/\/www.jetbrains.com\/help\/idea\/find-usages.html\" target=\"_blank\" rel=\"noopener\">Find Usages<\/a> (Alt+F7), and any that cared about the changed value of the <code>origProp<\/code> parameter need to be changed to use the new <code>databasePath<\/code> field instead.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15796\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-19-use-database-path-prev.png\" alt=\"Change callers to use the new database path\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-19-use-database-path.gif\" \/><\/p>\n<p>When I re-run the tests, I find that I&#8217;ve missed a step \u2013 the code that sets the database path in <code>ValidatedField<\/code> isn&#8217;t always reached in all cases, so I need to make sure I initialize the value in all cases.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15797\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-20-initialise-database-path-prev.png\" alt=\"Initialise database path correctly\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-20-initialise-database-path.gif\" \/><\/p>\n<p>Running the tests shows everything works now\u00a0\u2013 thank goodness! \u2013 so we can do some of the cleanup that should make our code easier to understand. We can delete the code that altered the <code>origProp<\/code> parameter:<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15788\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-21-delete-mutation-code-prev.png\" alt=\"Delete code that changes origProp\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-21-delete-mutation-code.gif\" \/><\/p>\n<p>Now that we&#8217;re not doing that nasty hack of using a <code>StringBuilder<\/code> to let us change what is effectively a String parameter, we can change this into a straightforward String.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15789\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-22-change-stringbuilder-to-string-prev.png\" alt=\"Change StringBuilder parameter to a String\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-22-change-stringbuilder-to-string.gif\" \/><\/p>\n<p>Finally, we can do some tidying up and replace all references to <code>origProp<\/code> and delete the parameter.<\/p>\n<p><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15790\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-23-remove-string-builder-prev.png\" alt=\"Remove StringBuilder from the callers\" width=\"1280\" height=\"720\" data-gif-src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-23-remove-string-builder.gif\" \/><\/p>\n<p>We should also check all the calling code, we can probably delete a lot of <code>StringBuilder<\/code> creation.<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-24-delete-string-builder.png\" rel=\"attachment wp-att-15784\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15784\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-24-delete-string-builder.png\" alt=\"Remove StringBuilder from the callers\" width=\"1390\" height=\"210\" \/><\/a><\/p>\n<p><strong>In Summary<\/strong><\/p>\n<p>At the end of all this, our <code>validateQuery<\/code> method is a <em>tiny<\/em>\u00a0bit smaller. It was 93 lines of code, and now it&#8217;s 74 lines. We achieved this simplification by trying to focus exclusively on where mutating values added complexity to the code. We:<\/p>\n<ol>\n<li>Stopped mutating the <code>pathElements<\/code> array that we were iterating over and introduced a <code>databasePathElements<\/code> collection. This meant we were no longer changing an array we were also trying to read. A nice bonus effect here is that we can use Java 8 streams functionality to produce our final database path rather than doing our own iteration (note: the performance of this is potentially not as fast as the original code. In the wider context of this application this doesn&#8217;t matter).<\/li>\n<li>Stopped setting a <code>hasTranslations<\/code> flag, which meant we could now move the mutation of the <code>MappedField mf<\/code> into a separate method. Moving this logic into its own method is one of the main contributors to the simplification of the larger method, but it&#8217;s still just a small step and may benefit from further refactoring.<\/li>\n<li>Stopped changing a parameter value, which was a hack around not being able to return multiple values from a method. We achieved this by introducing a new type to wrap both return values (the mapped field and the database path). This change slightly reduced the complexity of the method and let us remove code in places that call this method, but is also likely to be an area for further refactoring.<\/li>\n<\/ol>\n<p>The original code exhibits more than one smell.\u00a0By focusing on the problems caused by mutable values, we have taken a step towards making the code easier to understand. You&#8217;ll notice that we still make changes to values and to data structures, but what we&#8217;ve tried to do is a) limit mutation to smaller and easier to reason about sections, and b) make sure that the values that are changed or updated are not surprising ones.<\/p>\n<p>Symptoms<\/p>\n<ul>\n<li>Reassignment of variables<\/li>\n<li>Parameters altered to hold new values<\/li>\n<li>Passing around primitive wrapper classes (<code>Boolean<\/code>, <code>Integer<\/code> etc) or other wrappers (e.g. <code>StringBuilder<\/code>) in order to be able to alter the contents<\/li>\n<li>Difficult or impossible to extract smaller methods because multiple primitive values are being updated<\/li>\n<\/ul>\n<p>Possible solutions<\/p>\n<ul>\n<li>If the code is altering an array or collection that it is also reading from, consider having a second collection or array to track changes and leave the initial values unaltered. Reading and writing from the same collection, while safe in our specific example, can lead to unexpected results and concurrency issues.<\/li>\n<li>If it&#8217;s hard to tell which code branch a particular value came from, you may either want\u00a0to store several values with readable names instead of reusing a variable, or\u00a0move the code that changes the value into a separate method so that the changes are restricted to a small section of code.<\/li>\n<li>If input parameters are being altered, consider adding a new value to the object that is returned so it contains all the data that needs to be returned. If this isn&#8217;t possible, consider introducing a new type to wrap all the return values.<\/li>\n<li>Counters or boolean primitives may not be the best way to keep track of state within a method. Removing them or moving them into an appropriate domain class may make it easier to refactor\u00a0code into something simpler.<\/li>\n<\/ul>\n","protected":false},"author":360,"featured_media":0,"comment_status":"open","ping_status":"open","template":"","categories":[601],"tags":[3278,195],"cross-post-tag":[],"acf":[],"_links":{"self":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea\/25958"}],"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=25958"}],"version-history":[{"count":0,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea\/25958\/revisions"}],"wp:attachment":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/media?parent=25958"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/categories?post=25958"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/tags?post=25958"},{"taxonomy":"cross-post-tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/cross-post-tag?post=25958"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}