Start using Optional for return values when they can be null

Development
#21

Also, after reading a bit of documentation on the topic, it also important to understand where not to use Optional.
In the sample bellow, the first class presents some Optional anti-pattern. Where some alternatives are obviously better, I’m not sure if exposing Optional in the method and constructor parameters is always the best solution.

class FooWrong {
    private Optional<Bar> baz; // double indirection
    
    // makes the interface more (too?) complicated
    public FooWrong(Optional<Bar> baz)  { this.baz = baz; }

    public Optional<Bar> getBaz() { return this.baz; }

    // makes the interface more complicated
    public void setBaz(Optional<Bar> baz) { this.baz = baz; }
}

class FooOk1 {
    private Bar baz;
    public FooOk1(Bar baz) { this.baz = baz; }
    // re-mapping to optional
    public Optional<Bar> getBaz() { return Optional.ofNullable(this.baz); }
    public void setBaz(Bar baz) { this.baz = baz; }
}

class FooOk2 {
    private Bar baz;
    public FooOk2(Optional<Bar> baz) { this.baz = baz.orElse(null); }
    public Optional<Bar> getBaz() { return Optional.ofNullable(this.baz); }
    public void setBaz(Optional<Bar> baz) { this.baz = baz.orElse(null); }
}
#22

Object used in scripting APIs is a lot more than script services, it also includes everything returned by those script services. You seems to only have API used directly by a dev in mind but having getters potentially return null value is very common and those are often used in generic code (objects serialized to JSON, parsing of java beans, methods called by hibernate/JPA, etc.) for which you don’t know if it’s going to work (definitely not supported in JPA 2.2 from what I understood, Jackson does not support it by default and need some setup and it’s definitely not supported yet in xwiki-properties but it could be added).

Sure in some API it’s nice and should be used but it’s a case by case and I really don’t see the rush in enforcing it everywhere at all cost. Anyway my point is that even if you think you do you definitely don’t know if it’s going to work for all use cases.

Enforcing is is also not bringing much value since:

  • it’s impossible to have a tool checking it so there will be new APIs that won’t use it rule or not simply because it will be forgotten
  • XWiki is full of APIs returning null directly or exposing standard Java APIs in which Optional use is really far from being a rule so you can’t even rely on it to know if a method might return null or not

My point is that you can’t say let’s make it mandatory in script API right now and maybe we’ll add a helper sometime if it’s possible.

#23

From what I understand Vincent’s proposal is to make it a rule only for return values.

#24

Note: My proposal is to make it a best practice for return values that can be null. I haven’t researched not used enough optional elsewhere to have some strong opinion. But I don’t see anything not good in return values.

#25

So to be clear. I’ve proposed it as a best practice and didn’t mention any enforcement (EDIT: TBH I did say “must” at some point but that’s ofc provided there’s not a good reason to not use it). The reason I want use to consider it a best practice by default is that I would prefer that we don’t start to have 2 ways of defining APIs depending on the style of the API coder. Thus if someone proposes a new API and it has return values that can be null and Optional is not used, another dev reviewing it could ask to use Optional since we’d have agreed it’s our best practice. Then, there needs to be a good reason to not use it (like the Velocity use case before we do the uberspector thing for ex which seems easy enough - but impl. will tell).

It can also be said like this: always try to use Optional in return values for methods that can return null and don’t do it if there’s a good-enough reason (to be justified since it deviates from the best practice).

Do we agree with this?

#26

Basically if you see a new API that returns Optional then you know it can return a null. If you see a method that doesn’t return Optional then you can’t know if it can return null or not and you need to read the javadoc and if not mentioned in the javadoc then you need to read the sources or test it…

#27

Sure.

#28

Note: Optional need to be added to https://github.com/xwiki/xwiki-commons/blob/master/xwiki-commons-tools/xwiki-commons-tool-verification-resources/src/main/resources/checkstyle.xml#L109 otherwise it’s not really an encouragement to use it :slight_smile:

#29

Velocity scripting support implemented in https://jira.xwiki.org/browse/XCOMMONS-1930