It might seem obvious, but recently I saw a colleague use Optional in a weird way.
When I see a piece of code that seems to contain about 3 or more Optional.isPresent() if checks and Optional.get() method calls, I kind of panic.
To be absolutely fair, the code wasn't ready yet, so was going to change anyways.
So, anyways, before we had Optionals, we had to deal with NULL a lot.
The way to do this was as follows:
private boolean documentAccessAllowed() {
Information information = session.getInformation();
if (information != null) {
Document document = database.getDocument(information.getDocumentnumber());
if (document != null) {
if (!document.getOwner().equals(user)) {
Administrator administrator = database.getAdmininstrator(user.getName());
if (administrator == null) {
addErrorMessage("You are not allowed access to this document.");
return false;
}
}
}
}
return true;
}
I realize this code could be refactored using "Replace Nested Conditional with Guard Clauses", but some purists frown upon multiple return statements in a method. I don't mind. But let's continue with the example as is.
Then we got Optionals, yay! But, translating the above code verbatim causes my head to explode:
private boolean documentAccessAllowed() {
Optional<Information> information = session.getInformation();
if (information.isPresent()) {
Optional<Document> document = database.getDocument(information.get().getDocumentnumber());
if (document.isPresent()) {
if (!document.get().getOwner().equals(user)) {
Optional<Administrator> administrator = database.getAdmininstrator(user.getName());
if (administrator.isEmpty()) {
addErrorMessage("You are not allowed access to this document.");
return false;
}
}
}
}
return true;
}
This code is of course hardly ideal!
A better way is to use the flatMap and map and filter functions of Optional, which seems more concise, but requires a bit of a mental adjustment.
private boolean documentAccessAllowed() {
Boolean allowed = session.getInformation()
.map(Information::getDocumentnumber)
.flatMap(database::getDocument)
.filter(doc -> !doc.getOwner().equals(user))
.filter(doc -> database.getAdmininstrator(user.getName()).isEmpty())
.isEmpty();
if (!allowed) {
addErrorMessage("You are not allowed access to this document.");
}
return allowed;
}
The advice of a colleague of mine is that lambdas, even slightly trivial ones, are a bit hard to read, and making it a method with a good name helps clear things up immensely, like so:
private boolean documentAccessAllowed() {
boolean allowed = session.getInformation()
.map(Information::getDocumentnumber)
.flatMap(database::getDocument)
.filter(doc -> !isOwner(user, doc))
.filter(doc -> !isAdministrator(database, user))
.isEmpty();
if (!allowed) {
addErrorMessage("You are not allowed access to this document.");
}
return allowed;
}
The idea here is not to make the code short (although that most often helps) but make the code very easy to read and follow.
In the example above, there's a lot of use of negation, which I don't like either. (isEmpty(), !isAdmin, !isOwner) Sometimes shoehorning boolean expressions so you can put them in sequential filters is a bad idea. Perhaps the following is more clear:
private boolean documentAccessAllowed(Session session, Database database, User user) {
boolean allowed = session.getInformation()
.map(Information::getDocumentnumber)
.flatMap(database::getDocument)
.filter(doc -> isOwnerOrAdministrator(database, user, doc))
.isPresent();
if (!allowed) {
addErrorMessage("You are not allowed access to this document.");
}
return allowed;
}
private static boolean isOwnerOrAdministrator(Database database, User user, Document doc) {
return isOwner(user, doc) || isAdministrator(database, user);
}
Let me know if you have any suggestions, of which there are no doubt several, on how this can be improved in readability.
