{"id":25803,"date":"2017-08-08T15:50:18","date_gmt":"2017-08-08T15:50:18","guid":{"rendered":"https:\/\/blog.jetbrains.com\/idea\/?p=15467"},"modified":"2017-09-20T10:55:39","modified_gmt":"2017-09-20T10:55:39","slug":"code-smells-null","status":"publish","type":"idea","link":"https:\/\/blog.jetbrains.com\/ja\/idea\/2017\/08\/code-smells-null","title":{"rendered":"Code Smells: Null"},"content":{"rendered":"<p><em>This is part of a series\u00a0investigating\u00a0code that\u00a0looks suspicious\u00a0(&#8220;code smells&#8221;),\u00a0and exploring possible alternatives.<\/em><\/p>\n<ul>\n<li><strong>Code Smells: Null<\/strong><\/li>\n<li><a href=\"https:\/\/blog.jetbrains.com\/idea\/2017\/08\/code-smells-deeply-nested-code\/\">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><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>During my research on refactoring I&#8217;ve seen a number of patterns (smells) crop up again and again. None of these are particularly new, and there are <a href=\"https:\/\/martinfowler.com\/books\/refactoring.html\" target=\"_blank\" rel=\"noopener\">plenty<\/a>\u00a0of\u00a0<a href=\"https:\/\/martinfowler.com\/books\/r2p.html\" target=\"_blank\" rel=\"noopener\">books,<\/a>\u00a0<a href=\"https:\/\/refactoring.com\/\" target=\"_blank\" rel=\"noopener\">blogs<\/a>\u00a0and <a href=\"https:\/\/youtu.be\/y4_SJzNJnXU\" target=\"_blank\" rel=\"noopener\">videos<\/a> that cover smells and how to deal with them, but I wanted to demonstrate some specific, non-trivial examples and, of course, how IntelliJ IDEA may (or may not) be able to help you.<\/p>\n<p>The first problem I&#8217;ve being trying to counter is the use of nulls, particularly when it leads to null-checks scattered around the code.<\/p>\n<p>I thought\u00a0Java 8&#8217;s <a href=\"https:\/\/docs.oracle.com\/javase\/8\/docs\/api\/java\/util\/Optional.html\" target=\"_blank\" rel=\"noopener\"><code>Optional<\/code><\/a> should solve a lot of these problems. I assumed that a type that specifically states that it may be null, that is designed to let\u00a0you say what to do in these occasions, is exactly the right solution.<\/p>\n<p>However, things are never as simple as they seem, and I suspect that&#8217;s why there&#8217;s no magic &#8220;Optionalise my project&#8221; intention in IntelliJ IDEA.\u00a0 Rather disappointingly, this is an area that needs me, the developer, to think hard about what should be done.<\/p>\n<p><!--more--><\/p>\n<p>We have to come to terms with the fact that null means many things.\u00a0 It can mean:<\/p>\n<ol>\n<li>Value was never initialised (whether accidentally or on purpose)<\/li>\n<li>Value is not valid<\/li>\n<li>Value is not needed<\/li>\n<li>No such value exists<\/li>\n<li>Something went terribly wrong and something that should be there is not<\/li>\n<li>&#8230;probably dozens of other things<\/li>\n<\/ol>\n<p>You can assume some of these don&#8217;t apply to your code base if you&#8217;re a disciplined team with clear design goals &#8211; for example, having final fields that are always initialised in the constructor, using builders or factories to validate correct combinations before instantiation, not using nulls in the core of your application (i.e. pushing all checks to the edges) and so on.<\/p>\n<p><code>Optional<\/code> really solves only one of these cases, case 4. For example, you ask the database for a Customer with a given ID.\u00a0 Previously, you might have used <code>null<\/code> to represent this, but that could be ambiguous &#8211; does it mean the customer wasn&#8217;t found? Or that there&#8217;s a customer with that ID, but has no values? Or was <code>null<\/code> returned because the connection to the database failed?\u00a0By returning an empty <code>Optional<\/code>, you&#8217;ve removed the ambiguity &#8211; there&#8217;s no customer with that ID.<\/p>\n<p>In a normal, mature, code base that&#8217;s been worked on by numerous people, is it possible to tell what all our nulls mean, and what to do about them? \u00a0We should be able to make some progress by taking very tiny bites.<\/p>\n<p><strong>The Case Study<\/strong><\/p>\n<p>I&#8217;m using the <a href=\"https:\/\/github.com\/mongodb\/morphia\/\" target=\"_blank\" rel=\"noopener\">Morphia project<\/a> as an example in this blog post, as I have in previous talks and articles about refactoring. \u00a0This is\u00a0a great example project because it&#8217;s a) open source b) small enough to easily download and explore and c) mature enough to contain the sort of real life example code you probably have in your own applications.<\/p>\n<p><strong>The Smell: return null<\/strong><\/p>\n<p>I did a <a href=\"https:\/\/www.jetbrains.com\/help\/idea\/find-and-replace-in-path.html\" target=\"_blank\" rel=\"noopener\">Find in Path<\/a> for all the places where null is explicitly returned. \u00a0I have a theory that maybe we can change these methods into something that returns an <code>Optional<\/code>.<\/p>\n<div id=\"attachment_15696\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-01-FindInPath.png\" rel=\"attachment wp-att-15666\"><img aria-describedby=\"caption-attachment-15696\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15696 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-01-FindInPath.png\" alt=\"Find places that return null\" width=\"1234\" height=\"781\" \/><\/a><\/p>\n<p id=\"caption-attachment-15696\" class=\"wp-caption-text\">Results of &#8220;Find in Path&#8221; for &#8220;return null&#8221;<\/p>\n<\/div>\n<p><strong>Example 1: Converter.decode()<\/strong><\/p>\n<p>Given that lots of these *Converter classes seem to return a null value in the <code>decode<\/code> method, it seems reasonable that we might want to change the Converter superclass (an abstract class\u00a0called <a href=\"https:\/\/github.com\/mongodb\/morphia\/blob\/master\/morphia\/src\/main\/java\/org\/mongodb\/morphia\/converters\/TypeConverter.java\" target=\"_blank\" rel=\"noopener\"><code>TypeConverter<\/code><\/a>)\u00a0to return <code>Optional<\/code> here. \u00a0But a closer look at the code shows what&#8217;s actually happening: we see exactly the same pattern happening again and again &#8211; the method checks if a value passed in was null, and if so returns null:<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"Checking for null on decode\">@Override\r\npublic Object decode(final Class targetClass, final Object fromDBObject, final MappedField optionalExtraInfo) {\r\n    if (fromDBObject == null) {\r\n        return null;\r\n    }\r\n    \/\/...now do the decoding...\r\n}\r\n\r\n<\/pre>\n<p>The first question is can <code>fromDBObject<\/code> actually be null? The code is sufficiently complicated that it&#8217;s difficult to tell, but theoretically it seems plausible, since this is a value from the database.<\/p>\n<p>A quick search shows that all instances of this method are actually only called from one central place, so instead of making this check in\u00a021 different places, we can just do it in a single place and\u00a0from there on assume <code>fromDBObject<\/code> can no longer be null.<\/p>\n<p><strong>Solution: <code>@NotNull<\/code> parameter<\/strong><\/p>\n<p>My original assumption that we can use <code>Optional<\/code> here turned out to be wrong. \u00a0Instead, I&#8217;m going to change the method signature of <code>decode<\/code> to declare that <code>fromDBObject<\/code> cannot be null &#8211; I&#8217;ve opted to use the <a href=\"https:\/\/www.jetbrains.com\/help\/idea\/nullable-and-notnull-annotations.html\" target=\"_blank\" rel=\"noopener\">JetBrains annotations<\/a> to do this.<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"Declare fromDBObject NotNull\">public abstract T decode(Class&lt;T&gt; targetClass,\r\n                         @NotNull Object fromDBObject,\r\n                         MappedField optionalExtraInfo);\r\n<\/pre>\n<p>Then I move the null check (and subsequent null return) in the place where <code>fromDBObject<\/code> could actually be null, the single place that calls <code>decode<\/code>.<\/p>\n<div id=\"attachment_15697\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-02-MoveNullCheck.png\" rel=\"attachment wp-att-15667\"><img aria-describedby=\"caption-attachment-15697\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15697 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-02-MoveNullCheck.png\" alt=\"Add null check to call site\" width=\"1234\" height=\"185\" \/><\/a><\/p>\n<p id=\"caption-attachment-15697\" class=\"wp-caption-text\">Moving the null check to the single call site where fromDBObject can be null<\/p>\n<\/div>\n<p>It makes the call site a bit more untidy, but constrains this logic to a single place instead of scattered through all implementations. Now, we can either document the fact that <code>fromDBObject<\/code> will not be null or rely on the annotation to do this for us, so we never have to perform this null check in the Converter implementations.<\/p>\n<p>Next we can tidy up and remove all the duplicated code. Here IntelliJ IDEA can help us. \u00a0If we go back to the abstract method in <code>TypeConverter<\/code> that we annotated with the <code>@NotNull<\/code> parameter, we are given a warning:<\/p>\n<div id=\"attachment_15698\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-03-OverriddenNotAnnotated.png\" rel=\"attachment wp-att-15668\"><img aria-describedby=\"caption-attachment-15698\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15698 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-03-OverriddenNotAnnotated.png\" alt=\"Overridden parameters are not annotated\" width=\"1234\" height=\"107\" \/><\/a><\/p>\n<p id=\"caption-attachment-15698\" class=\"wp-caption-text\">Warning that implementations don&#8217;t annotate this parameter<\/p>\n<\/div>\n<p>The\u00a0quick fix suggested when we use Alt+Enter on this warning lets us apply this annotation to all the implementations of this method in one easy step, so let&#8217;s do that.<\/p>\n<div id=\"attachment_15699\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-04-QuickFixImplementations.png\" rel=\"attachment wp-att-15669\"><img aria-describedby=\"caption-attachment-15699\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15699 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-04-QuickFixImplementations.png\" alt=\"Quick fix to apply the annotation to all implementations\" width=\"1234\" height=\"164\" \/><\/a><\/p>\n<p id=\"caption-attachment-15699\" class=\"wp-caption-text\">Use the quick fix to apply to all implementations<\/p>\n<\/div>\n<p>Our VCS log shows\u00a0that all the Converter implementations have been updated<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-05-ConvertersUpdated.png\" rel=\"attachment wp-att-15670\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone wp-image-15700 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-05-ConvertersUpdated.png\" alt=\"All Converters updated\" width=\"1234\" height=\"450\" \/><\/a><\/p>\n<p>This has added the <code>@NotNull<\/code> annotation to the <code>decode<\/code> method&#8217;s parameter, but we still haven&#8217;t tracked down and removed all the duplicate null-check code. \u00a0The good news is that now we&#8217;ve said <code>fromDBObject<\/code> cannot be null, we can use the inspections to find\u00a0all of our redundant null checks and remove them.<\/p>\n<p>One way to do this is to navigate into an implementation that we know has the check (in this case I chose <a href=\"https:\/\/github.com\/mongodb\/morphia\/blob\/master\/morphia\/src\/main\/java\/org\/mongodb\/morphia\/converters\/StringConverter.java\" target=\"_blank\" rel=\"noopener\">StringConverter<\/a>) to see what warning IntelliJ IDEA gives me.<\/p>\n<div id=\"attachment_15701\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-06-NullCheckNotNeeded.png\" rel=\"attachment wp-att-15671\"><img aria-describedby=\"caption-attachment-15701\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15701 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-06-NullCheckNotNeeded.png\" alt=\"Null check not needed as value is never null\" width=\"1234\" height=\"160\" \/><\/a><\/p>\n<p id=\"caption-attachment-15701\" class=\"wp-caption-text\">Null check not needed as value is never null<\/p>\n<\/div>\n<p>This\u00a0turns out to be triggered by the &#8220;<a href=\"https:\/\/blog.jetbrains.com\/idea\/2009\/04\/avoiding-assert-statements-with-constant-conditions\/\">Constant conditions &amp; exceptions<\/a>&#8221; inspection. We can run just this inspection over the code base to find other examples\u00a0easily, using <a href=\"https:\/\/www.jetbrains.com\/help\/idea\/running-inspection-by-name.html\" target=\"_blank\" rel=\"noopener\">Ctrl+Shift+Alt+I and typing the inspection name<\/a>.<\/p>\n<div id=\"attachment_15702\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-07-RunInspectionByName.png\" rel=\"attachment wp-att-15672\"><img aria-describedby=\"caption-attachment-15702\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15702 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-07-RunInspectionByName.png\" alt=\"Run inspection by name\" width=\"1234\" height=\"587\" \/><\/a><\/p>\n<p id=\"caption-attachment-15702\" class=\"wp-caption-text\">Using Ctrl+Shift+Alt+I to run an inspection by name<\/p>\n<\/div>\n<p>This returns more results than just my Converters, but by grouping the results by directory I can see all the Converter implementations easily<\/p>\n<p><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-08InspectionResults.png\" rel=\"attachment wp-att-15673\"><img decoding=\"async\" loading=\"lazy\" class=\"alignnone size-full wp-image-15673\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-08InspectionResults.png\" alt=\"Inspection results\" width=\"1411\" height=\"909\" \/><\/a><\/p>\n<p>I can use the preview window on the right to check which code has been flagged, and see what IntelliJ IDEA suggests doing with each problem. \u00a0After scanning through and making sure this is the null check I&#8217;m looking for, I select the converters directory\u00a0and press the &#8220;Simplify boolean expression&#8221; button.<\/p>\n<p>To sanity check the changes, I go into the <a href=\"https:\/\/www.jetbrains.com\/help\/idea\/local-changes-tab.html\" target=\"_blank\" rel=\"noopener\">VCS Local Changes<\/a> window and use Show Diff (Ctrl+D) to check the changes that were applied to the 36 files there.<\/p>\n<div id=\"attachment_15674\" style=\"width: 2145px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-09Diff.png\" rel=\"attachment wp-att-15674\"><img aria-describedby=\"caption-attachment-15674\" decoding=\"async\" loading=\"lazy\" class=\"size-full wp-image-15674\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-09Diff.png\" alt=\"Using the diff view to sanity check changes\" width=\"2135\" height=\"859\" \/><\/a><\/p>\n<p id=\"caption-attachment-15674\" class=\"wp-caption-text\">Using the diff view to sanity check changes<\/p>\n<\/div>\n<p>I can even use the diff view to make small edits if I want &#8211; in this case I use Ctrl+Y to remove the unnecessary empty lines 26 and 27 of my file (the file on the right). By using Alt+Right to check all the files that have been changed, I find an example where the null check wasn&#8217;t removed (this was a test converter and therefore was not in the converters directory), but I can easily use quick fixes even in the diff view to remove it.<\/p>\n<p>This sanity check\u00a0gave me peace of mind about the automatic changes IntelliJ IDEA made, and let me make some additional tweaks\u00a0too. The last thing to do is run all the tests, and when they pass (hurrah!) now seems like a really good time to commit all these changes.<\/p>\n<p>If you&#8217;re uncomfortable adding a new library to document\u00a0that a parameter can&#8217;t be null and make the IDE warn you if you pass in null values, you can always remove the <code>@NotNull<\/code> just before committing &#8211; after all, it&#8217;s done its job now. In this case, you should at least update the Javadoc to state\u00a0that the parameter is assumed to be not null.<\/p>\n<p><strong>Example 2: Converter.encode()<\/strong><\/p>\n<p>Earlier we saw that the <code>decode<\/code> method was only one of several that explicitly returned null values. \u00a0Now we&#8217;ve dealt with that, we can take a look at another example.<\/p>\n<p>The <code>encode<\/code> method on the same <code>TypeConverter<\/code> class can also return null. \u00a0But unlike <code>decode<\/code>, there are valid times when the converter implementations explicitly return null:<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"CharacterConverter.encode()\">@Override\r\npublic Object encode(Object value, MappedField optionalExtraInfo) {\r\n    return value.equals(&#039;\\0&#039;) ? null : String.valueOf(value);\r\n}\r\n<\/pre>\n<p><strong>Solution: <code>Optional<\/code><\/strong><\/p>\n<p>This is a good place for <code>Optional<\/code>&#8211; we&#8217;re explicitly stating that there are some cases where the encode method does not return a value.\u00a0<code>Optional.empty()<\/code> seems like a good representation of this fact.\u00a0 So I change <code>TypeConverter.encode()<\/code> method\u00a0to return an <code>Optional<\/code> instead.\u00a0 It makes all the implementations a little more untidy, as things need to be wrapped in an <code>Optional<\/code>, but it is more explicit about the fact that this method sometimes does not return a value. I&#8217;d love to show you IntelliJ IDEA magic that does this for you (in some cases, <a href=\"https:\/\/www.jetbrains.com\/help\/idea\/type-migration.html\" target=\"_blank\" rel=\"noopener\">Type Migration<\/a> will work, but not in my example) but I made this change the hard way &#8211; I changed the return type of the super class to <code>Optional<\/code> and fixed all the places that broke. The good news is\u00a0that once I&#8217;d changed the method signature to return an <code>Optional<\/code>, IntelliJ IDEA did offer a quick fix to wrap my return values in an <code>Optional<\/code>.<\/p>\n<div id=\"attachment_15703\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-10-WrapReturnWithOptional.png\" rel=\"attachment wp-att-15682\"><img aria-describedby=\"caption-attachment-15703\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15703 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-10-WrapReturnWithOptional.png\" alt=\"Wrap value in Optional\" width=\"1234\" height=\"357\" \/><\/a><\/p>\n<p id=\"caption-attachment-15703\" class=\"wp-caption-text\">Wrap value in Optional<\/p>\n<\/div>\n<p>Similarly, null values can also be replaced<\/p>\n<div id=\"attachment_15704\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-11-ReplaceNullWithOptionalEmpty.png\" rel=\"attachment wp-att-15681\"><img aria-describedby=\"caption-attachment-15704\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15704 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-11-ReplaceNullWithOptionalEmpty.png\" alt=\"Return Optional.empty() instead of null\" width=\"1234\" height=\"372\" \/><\/a><\/p>\n<p id=\"caption-attachment-15704\" class=\"wp-caption-text\">Return Optional.empty() instead of null<\/p>\n<\/div>\n<p>Now we&#8217;re returning an <code>Optional<\/code>, we have to work with this new type at the call site. \u00a0In my case,\u00a0I actually got no compiler warnings as my return type was originally an <code>Object<\/code>, and obviously an <code>Optional<\/code> is also an <code>Object<\/code> (note: this is why we shouldn&#8217;t use raw Object types, because it basically removes the benefits of having a strongly typed language. This code would have been better with <a href=\"https:\/\/docs.oracle.com\/javase\/tutorial\/java\/generics\/\" target=\"_blank\" rel=\"noopener\">Generics<\/a>). I just have to change the one place that calls the <code>encode<\/code> method to unwrap the <code>Optional<\/code>. \u00a0I&#8217;m going to do something that&#8217;s pretty nasty and use <code>orElse(null)<\/code>, but since this is the same behaviour the original methods were doing anyway, and it&#8217;s restricted to one place in the code, I&#8217;m happy with this &#8211; I can flag it as tech debt and we can gradually deal with the issue rather than\u00a0chasing it all around the code.<\/p>\n<div id=\"attachment_15705\" style=\"width: 1244px\" class=\"wp-caption alignnone\"><a href=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-12-DealWithOptionalReturn.png\" rel=\"attachment wp-att-15687\"><img aria-describedby=\"caption-attachment-15705\" decoding=\"async\" loading=\"lazy\" class=\"wp-image-15705 size-full\" src=\"https:\/\/blog.jetbrains.com\/wp-content\/uploads\/2017\/08\/idea-12-DealWithOptionalReturn.png\" alt=\"Dealing with the optional return\" width=\"1234\" height=\"57\" \/><\/a><\/p>\n<p id=\"caption-attachment-15705\" class=\"wp-caption-text\">This preserves the original behaviour, but should be considered tech debt to be addressed later.<\/p>\n<\/div>\n<p>In\u00a0tests that call the <code>encode<\/code> method I simply updated them to call <code>encode(o).get()<\/code> &#8211; usually this is unsafe, but a) the tests should not return an empty <code>Optional<\/code> and b) if they do, they&#8217;ll fail and that&#8217;s correct. In fact in my case this highlights that there are no tests for the case where no value is returned, so I should add new tests for empty <code>Optional<\/code> returns.<\/p>\n<p>Changing your API to use <code>Optional<\/code> is often a non-trivial task, but in cases like this one it can really help clarify the intent &#8211; instead of returning nulls when there&#8217;s no valid value, it will return you an empty <code>Optional<\/code> and you can declare what to do &#8211; return an alternate value, throw an Exception, or perform some other operation. Beware though, this can get hairy.<\/p>\n<p>Note: This example was OK, but there are other methods in this code that are perfect for returning <code>Optional<\/code>, but the upstream effect of applying this change was huge &#8211; the methods are called in multiple places and the return types used in all sorts of very complex methods, making it nearly impossible to track down the right place to test the Optional and unwrap it vs letting it propagate around the code. The lesson I have learnt is apply this if you can limit the impact to one or two call sites and deal with the <code>Optional<\/code> return type immediately.<\/p>\n<p><strong>Example\u00a03: Mapper.getId()<\/strong><\/p>\n<p>Here&#8217;s another example of null returns:<\/p>\n<pre class=\"EnlighterJSRAW\" data-enlighter-language=\"java\" data-enlighter-title=\"getId\">    public Object getId(final Object entity) {\r\n        Object unwrapped = entity;\r\n        if (unwrapped == null) {\r\n            return null;\r\n        }\r\n        unwrapped = ProxyHelper.unwrap(unwrapped);\r\n        try {\r\n            return getMappedClass(unwrapped.getClass()).getIdField().get(unwrapped);\r\n        } catch (Exception e) {\r\n            return null;\r\n        }\r\n    }<\/pre>\n<p>This is a great example of where null means two different things. The first null means we were given a null input, therefore we give you a null output.\u00a0 Seems fairly valid, if a little unhelpful. The second null means &#8220;an error happened and I couldn&#8217;t give you a value, so here, have null&#8221;.\u00a0 Presumably a null ID is a valid response, given the first case, but it would probably be more helpful to the caller to know what the error was and deal with it accordingly. \u00a0Even if the Exception is such that the caller cannot deal with it,\u00a0it&#8217;s pretty unfriendly to catch the Exception and return null, especially when the Exception isn&#8217;t even logged.\u00a0 This is a really good way to hide genuine problems. \u00a0Exceptions should be handled where they&#8217;re found, or passed on in some useful way, they should not be swallowed unless you really <em>really<\/em> know it&#8217;s not an error.<\/p>\n<p>So this <code>null<\/code> means &#8220;something unexpected happened and I don&#8217;t know what to do about it, so I&#8217;m going to give you null and hope that&#8217;s OK&#8221;, which is not at all clear to the caller.\u00a0 These cases should definitely be avoided, and <code>Optional<\/code> is not going to be your solution. The solution is to implement much clearer error handing.<\/p>\n<p><strong>In summary<\/strong><\/p>\n<p>Null is a particularly difficult problem. The main problem with null is we don&#8217;t know what it means. It&#8217;s an absence of a value, but this could be for any number of reasons. Using null is not a useful way to flag what any of these reasons are, since by its very definition it has no value, no meaning.<\/p>\n<p>Symptoms:<\/p>\n<ul>\n<li>Null checks extensively used throughout the application code, without it being clear to you, the developer, what null really signifies or if the value can really be null at all.<\/li>\n<li>Explicit <code>return null<\/code><\/li>\n<\/ul>\n<p>Possible Solutions:<\/p>\n<ul>\n<li><code>Optional<\/code>.\u00a0 Not a solution to everywhere you find a null.\u00a0 But if a method is deliberately returning a null value to mean &#8220;not found&#8221; or &#8220;I don\u2019t have one of those&#8221;, this\u00a0is a good candidate for returning an <code>Optional<\/code>.<\/li>\n<li><code>@NotNull\/@Nullable<\/code>. One of the problems with complex code bases is understanding what are actually valid values. If you&#8217;re checking for null parameters, try putting <code>@NotNull<\/code> on the signature and seeing if null values are ever passed.\u00a0 If nulls are never passed, you can remove the null check.\u00a0 If you&#8217;re uncomfortable adding a dependency just to use the <code>@NotNull<\/code> annotation, you can apply the annotation temporarily, get IntelliJ IDEA to tell you if there are any problems, fix up if necessary and run all your tests to make sure everything works.\u00a0 If it&#8217;s all good, you can remove the annotation, add a Javadoc comment (not as safe as the annotation, but still enormously useful for developers who call or maintain the method) and commit\u00a0the updated code.<\/li>\n<li>Exception handling. Returning null for an Exceptional case is very unfriendly, and unexpected from the caller&#8217;s point of view.\u00a0 It can lead to <code>NullPointerException<\/code> further down the line, or at least more of the pervasive null checks (without a clear idea of <em>why<\/em> the value is null).\u00a0 Throwing a descriptive Exception is much more valuable for those downstream.<\/li>\n<li>Sometimes, it&#8217;s fine. \u00a0Field-level nulls (where it should be well understood what can be null and why) are examples where null may be perfectly acceptable.<\/li>\n<\/ul>\n","protected":false},"author":360,"featured_media":0,"comment_status":"open","ping_status":"open","template":"","categories":[601],"tags":[3278,756,2993,3274,195],"cross-post-tag":[],"acf":[],"_links":{"self":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea\/25803"}],"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=25803"}],"version-history":[{"count":0,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/idea\/25803\/revisions"}],"wp:attachment":[{"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/media?parent=25803"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/categories?post=25803"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/tags?post=25803"},{"taxonomy":"cross-post-tag","embeddable":true,"href":"https:\/\/blog.jetbrains.com\/ja\/wp-json\/wp\/v2\/cross-post-tag?post=25803"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}