Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decide if we will ever return null from a function. #10

Open
lbergelson opened this issue Jul 6, 2018 · 5 comments
Open

Decide if we will ever return null from a function. #10

lbergelson opened this issue Jul 6, 2018 · 5 comments

Comments

@lbergelson
Copy link
Member

How do we feel about returning nulls? It's gross, but I'm not totally convinced that optionals are actually less gross in java. Optional has the benefit of documenting the fact that something may not be present where null can be a surprise. The have the downside that it's wrapping the element in a new object which is potentially expensive for critical things.

I think we have 3 options:

  1. Allow methods to return null. This is error prone in some ways, but it's familiar, it's not very verbose, and null checks are very inexpensive.
  2. Don't allow methods to return null, instead throw and have hasX methods alongside getX methods. This has the advantage of avoiding NPE but is similarly error prone if people forget to check the hasX. It could also be complicated to efficiently implement hasX if getX is something other than returning a field.
  3. Use Optional as a return value instead of nulls. This is nicer in a lot of ways, but it's a big change, the syntax in java is only marginally less verbose than null pointer checks, and it introduces additional boxing overhead for all calls to the method, which is likely much more expensive than null checks. If java supported value types this would be much more appealing since the overhead could be avoided.
@magicDGS
Copy link
Member

magicDGS commented Jul 7, 2018

I prefer to never return nulls, so either 2 or 3 fits that criteria. But in my opinion, Optional is the best option at least for some cases:

  • For getters that can return missing values, such as variant attributes (FORMAT/INFO), it is kind of interesting to distiguish between them. Optional is great for it: a missing value is returned as the string representation (.) or special value (-1?), and completely undefined as an Optional.empty()
  • Methods returning a boxed primitive (e.g., int) that can be null can return an Optional instead, because there are implementations storing the primitive (e.g. OptionalInt) that would have teh same performance as boxing (e.g., Integer).

I suggest never return null, but have a mixed approach depending on the case: for example, the previous cases are better represented by returning an Optional, but cases where it would become a performance issue can use the hasX/getX implementation. On the other hand, there are values where missing can be represented in a different way (e.g., getContig() can return an empty string if there is no contig or the special * to represent it; better than a hasX method, it can be directly compared with == by forcing the static object to be returned without copying).

Does this mixed strategy works for you?

@tfenne
Copy link
Member

tfenne commented Jul 10, 2018

I'm nearly in agreement with @magicDGS, but not quite. My view is that null is parallel to exception. What I mean is that exceptions should be used for exceptional conditions and not expected results. Similarly for null, I think if the value being accessed is optional and can reasonably be expected to be missing, then Optional is preferred to null. But in cases where something really shouldn't be null in 99.99% of cases, but occasionally can in weird situations, I think it's preferable to return null since using Optional gives the wrong impression.

@magicDGS
Copy link
Member

So from the comments here, what I imagine now as a safe design:

  1. Avoid null in most of the cases.
  2. Use a constant in case there is a defined value the SAM/VCF/whatever specs (but not magic numbers/values). E.g., missing reference can be * (emty String is also a possibility, as usually empty is not a valid name).
  3. Optional for cases where we can expect that something is not present: attributes in VCF or SAM records
  4. null when none of this cases fits

Some return methods can distinguish in the API between missing in the record and missing value defined (e.g., VCF . missing value). In that case, this could be defined as 3+2 (return optional; not present was missing in the record; present but with a special value is explicitly missing in the record) or 3+4 (return empty optional if explicitly missing; null in case of not present at all).

I found this Wiki page from guava quite good for this discussion too: https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained

@magicDGS
Copy link
Member

More info about null to impove discussion: https://www.yegor256.com/2014/05/13/why-null-is-bad.html

@magicDGS
Copy link
Member

What's about Bean Validation for specifying this and other constraints?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants