public interface UserProperties extends ConfigurationSource
{
/**
* @return true if the user is configured to display hidden documents in the wiki
*/
boolean displayHiddenDocuments();
/**
* @return true if the user is active in the wiki. An active user can log in.
*/
boolean isActive();
/**
* @return the first name of the user or null if not set
*/
Optional<String> getFirstName();
/**
* @return the last name of the user or null if not set
*/
Optional<String> getLastName();
/**
* @return the email address of the user and null if not set
*/
Optional<String> getEmail();
/**
* @return the type of the user (simple user, advanced user)
* @see <a href="https://bit.ly/37TUlCp">user profile</a>
*/
UserType getType();
/**
* @return the default editor to use when editing content for this user (text editor, wysiwyg editor)
*/
Editor getEditor();
...
That’s definitely a great idea. When I noticed that XWiki codebase doesn’t use it, I was kinda surprised. So yes, this is a great move.
Also, give this guarantee that if a method returns T, not Optional<T>, then such a function must NOT return null for a T, as it defeats the purpose of returning Optional<U> for some other API. In the example, getType() and getEditor must not return null in any case.
In other words, no function shall return null.
I wish I could also contribute refactoring the codebase.
Which means we won’t be able to use the following (AFAIK, since it would throw a NoSuchElementException which is not handled in a special way by Velocity):
One thing that is very important to understand is that Optional.get is unsafe.
Using this method without checks only replaces NullPointerException by NoSuchElementException.
The use of isPresent before calling get, or even using the other provided operations (e.g. orElseGet, map) must be done as much as possible.
Stuff like ifPresent and orElse are nice helpers (but totally unusable in Velocity for example) but I’m not really convinced it’s enough to make it mandatory (definitely a bad idea for script services IMO). Note that I did not had to manipulate Optional in any of the Java libraries I used so far.
It not just that, it’s a way to cleanly express to api user that some method can return null (without them having to rely on not up todate or incomplete javadoc). And yes it allows to write stuff without having checking for null everywhere. You can chain lambdas nicely and remove the need for IFs.
TBH after I did my research I haven’t seen any counter argument to not using Optional for return values that can be null. There are arguments to not use it elsewhere but I haven’t see that for returns.
Now there’s velocity for which we need to decide. It’s not a real problem since using isPresent() or get() will work fine. It just makes it a bit more complex/unnatural to use. Hence, my idea that we could have an uberspector (or modify our existing one) that would call the get() automatically if what is returned is Optional, and return null if the value is null. That should be doable.
