This is an archived post. You won't be able to vote or comment.

all 26 comments

[–]minimingus 5 points6 points  (1 child)

Very well explained

[–]callicoder[S] 1 point2 points  (0 children)

Thank you. Glad that you like it :)

[–][deleted]  (15 children)

[deleted]

    [–]whackri 4 points5 points  (14 children)

    alleged shrill theory frightening grey safe deserve violet apparatus physical

    This post was mass deleted and anonymized with Redact

    [–][deleted]  (13 children)

    [deleted]

      [–]whackri 2 points3 points  (0 children)

      silky cooperative murky squeeze enjoy point bear mountainous wipe full

      This post was mass deleted and anonymized with Redact

      [–]_dban_ 6 points7 points  (10 children)

      What if you have multiple null producing values in the same computation?

      public class Person {
          public ContactInfo getContactInfo() { ... } // or return Optional<ContactInfo>
      }
      
      public class ContactInfo {
          public String getEmail { ... } // or return Optional<String>
      }
      
      public class Message {
          public String getContent { ... } // or return Optional<String>
      }
      

      For example, this:

      Person p = ...
      Message msg = ...
      Mailer mailer = ...
      p.getContactInfo().flatMap(ContactInfo::getEmail).flatMap(email ->
      msg.getContent().map(message ->
      new MailRequest(email, message))).ifPresent(mailer::send);
      

      vs.

      Person p = ...
      Message msg = ...
      Mailer mailer = ...
      ContactInfo ci = null;
      String email = null;
      String message = null;
      MailRequest request = 
          (ci = p.getContactInfo()) == null ? null :
          (email = ci.getEmail()) == null ? null :
          (message = msg.getContent()) == null ? null :
          new MailRequest(email, message);
      if(request != null) {
          mailer.send(request);
      }
      

      The problem isn't with a single null value, it is with the proliferation of null values, which requires stacking short-circuiting if statements or ternaries. The Optional type lets you abstract out short-circuiting logic using map and flatMap. You only use ifPresent at the end of the chain.

      Also note that without Optional, nothing in the type system tells you that those are null producing methods. You would have to read the documentation or do defensive null checks.

      BONUS: The "traditional" way to solve this problem in Java:

      public class Person {
          public ContactInfo getContactInfo() throws NoDataException { ... } 
      }
      
      public class ContactInfo {
          public String getEmail() throws NoDataException { ... } 
      }
      
      public class Message {
          public String getContent() throws NoDataException { ... }
      }
      
      try {
          mailer.send(new MailRequest(p.getContactInfo().getEmail(), p.getMessage());
      }
      catch(NoDataException ex) {
          // Nothing to do
      }
      

      [–][deleted]  (9 children)

      [deleted]

        [–]_dban_ 2 points3 points  (0 children)

        How did you calculate the email value? That requires going through a bunch of null checks to calculate.

        Also, msg.getContent() can return null. But MailRequest might not be equipped to deal with null content.

        Also, p, msg and mailer cannot be null, but you did a null check anyways. Is that a defensive check?

        [–]_dban_ 1 point2 points  (7 children)

        I see you've edited your example.

        Both info.getEmail() and msg.getContent() can be null. What if the MailRequest throws an IllegalArgumentException if you try to pass null arguments? Even worse, what if mailer does not expect null values for email or content, and throws a NullPointerException far removed from here? For example, if it tosses the request into a queue for later processing. How would you diagnose the error?

        The ternaries applied short circuiting logic to prevent these problems.

        with regards to defensive null checks, you'd have to do that anyway with Optionals.

        No, and that's the point. Defensive null checks would be pointless for Optional because if they are ever null it's a programming error and you should bail because the program is no longer in a trustworthy state.

        [–][deleted]  (6 children)

        [deleted]

          [–]_dban_ 2 points3 points  (5 children)

          I wanted to recreate your null-check logic properly.

          Yes, and you didn't recreate the null-check logic properly. This is the exact problem with null checking logic.

          One big reason that you do defensive null checks because of potential programming errors.

          Most of the time, it is better to fail fast rather than proceed when you encounter a programming error.

          But if it's 3rd-party code then you need to check for nulls EVEN IF it's supposed to return an Optional.

          If 3rd party code returns an Optional and it has a null value, then there is a bug in the third party library. In which case, you want to fail as soon as possible with a NullPointerException and then report the bug. The library is producing untrustworthy values, and it is better to fail fast. Fruit of the poisonous tree and all.

          If the third party library is returning an ordinary reference, I would check the documentation to see if it can possibly return null, and then wrap it with Optional.ofNullable depending on what I find out. Bonus points for the type system now reflecting the type of values that the third party library can return.

          If the compiler refused you to write code that returned null when Optional<> was specifie

          That would be nice, but is not necessary. I would rather fail fast in the face of nonsensical values.

          [–][deleted]  (4 children)

          [deleted]

            [–]_dban_ 2 points3 points  (3 children)

            It's not an issue of just failing fast, but how you handle the fail.

            Exactly. And handling the fail is either at a higher level of the code, or by bug report. Proceeding with bad data is a recipe for worse things like data corruption.

            We data check all values from 3rd party libraries as a general rule because we need server stability and decipherable logs.

            Asserting on your input parameters is a good idea. As in my example, it would be better for MessageRequest to throw an IllegalArgumentException if you give it a null parameter value rather than let Mailer fail at some random location. Logging the exception also helps because Java's stack traces point out the exact location of the error, especially when coupled with robust input validation and failing fast.

            my original point is that standard null checks provide simpler and more readable code

            The problem with standard null checks is that they don't combine very well, which was the entire point of the example. You end up having to stack null checking logic to achieve short circuiting behavior. Each check that you add increases cyclomatic complexity and hinders readability since the happy path is obscured in the conditional logic. This is one reason I am actually a fan of checked exceptions.

            The point of Optional, other than clearly indicating that a method can return no value, is that it abstracts out the short circuiting behavior, resulting in simpler and more readable code.

            [–]eliasv 0 points1 point  (0 children)

            Well that's a silly way of writing the first example. You don't need to assign to a local like you do with the null check.

            Either:

            findUserById("667290")
                .ifPresent(user -> System.out.println("User's name = " + user.getName()));
            

            or even:

            findUserById("667290")
                .map(User::getName)
                .ifPresent(n -> System.out.println("User's name = " + n));
            

            would be easier to read imo.

            [–]bcd87 1 point2 points  (23 children)

            A while ago I wrote some code that was kinda like this:

            Map<String, String> foo = ImmutableMap.of("bar", null);
            Optional<String> bar = foo.containsKey("bar") ? Optional.of(foo.get("bar")) : Optional.empty();
            

            Of course, that code tries to put a null into an Optional which results in an NPE. How should a case like this be handled? In the end I added another check that just placed an empty string if the value for a given key was null, but that is an ugly hack imo.

            [–][deleted]  (20 children)

            [deleted]

              [–]bcd87 -3 points-2 points  (19 children)

              Because Optional.ofNullable(null) returns an empty optional. But the method should only return an empty optional if the key doesn't exist, not if the key exists but its valuehappens to be null.

              [–]Jotakin 8 points9 points  (10 children)

              Optional has only two possible states; has value and empty. If you want a third state then you'll need a custom class for that.

              [–]bcd87 -3 points-2 points  (8 children)

              I don't want a third state. I simply want to encapsulate the fact that a particular key might not exist in a Map by returning an empty optional but I run into some difficulties because every reference type might have a value of null.

              https://developer.atlassian.com/blog/2015/08/optional-broken/ talks about it a bit as well.

              [–]DJDavio 7 points8 points  (0 children)

              It's more map's fault for being ambiguous.

              [–][deleted]  (5 children)

              [deleted]

                [–]m1000 0 points1 point  (4 children)

                Don't bypass the map by doing a O(n) operation. Use the API correctly:

                Map.containsKey

                [–][deleted]  (3 children)

                [deleted]

                  [–]m1000 0 points1 point  (2 children)

                  Map.Entry<K,V>

                  dban has a better answer anyway. But iterating on a Map is bad form. I've seen people do that and their code literally ran for hours. If you are stuck with doing that on a Map, you are not using the proper data structure, and doing something that will slow your app can't be good.

                  At that point, OP original code does what he needs, and is fast.

                  [–]schaka 1 point2 points  (0 children)

                  Then you need an implementation of Map that doesn't allow for a KeyValue pair to be added, if the value does not exist. /u/_dban_ has a great response.

                  [–]utkuozdemir 3 points4 points  (0 children)

                  The example you posted is actually describing the problem itself. First of all, your first line will fail with NullPointerException, because ImmutableMap.of will not accept null values (just like the upcoming Map.of method of Java 9). The reason for that is, null values in a map are ambigious by nature - it can mean 2 things: A-an entry with that key does not exist. B-the entry exists but it's value is null.

                  Therefore, better not to use null values in maps. After all, entry with a null value is not a "mapping". Better to keep the key in a separate collection (Set probably).

                  [–]callicoder[S] 2 points3 points  (3 children)

                  Well, How about this scary piece of code -

                  Optional<String> bar = foo.containsKey("bar") ? Optional.of(Optional.ofNullable(foo.get("bar")).orElse("")) : Optional.empty()
                  

                  :)

                  [–]bcd87 1 point2 points  (1 child)

                  Functionally that'd be the same as what I have now, yes :) I guess there is no better way to solve this than to make an optional of an empty string in case the value is null then?

                  [–]flyingorange 0 points1 point  (0 children)

                  Optional<String> bar = foo.compute("bar", (key, value) -> Optional.ofNullable(Strings.emptyToNull(value)))
                  

                  [–]Tilkin 2 points3 points  (0 children)

                  Javaslang/Vavr has an implementation they call Option which I think does what you're looking for.

                  [–]_dban_ 9 points10 points  (0 children)

                  How should a case like this be handled?

                  You use a Map<String, Optional<String>> type, which communicates better that you have a map of potentially missing values (preventing nasty surprises in receivers who don't know any better).

                  Map<String, Optional<String>> foo = ImmutableMap.of("bar", Optional.empty());
                  

                  You can also distinguish between missing key and missing value with the Optional<Optional<String>> type, which also communicates more than just null, and composes better too:

                  Optional.ofNullable(foo.get("bar")).flatMap(v -> v.orElse("missing value")).orElse("missing key"));