Secondary resource access with Optional #782
Conversation
| @@ -6,5 +6,9 @@ | |||
|
|
|||
| Optional<RetryInfo> getRetryInfo(); | |||
|
|
|||
| <T> T getSecondaryResource(Class<T> expectedType, String... qualifier); | |||
| default <T> Optional<T> getSecondaryResource(Class<T> expectedType) { | |||
There was a problem hiding this comment.
Not sure I like this change…
| Optional<Tomcat> tomcatValue = context.getSecondaryResource(Tomcat.class); | ||
| if (tomcatValue.isEmpty()) { | ||
| throw new IllegalStateException("Cannot find Tomcat " + webapp.getSpec().getTomcat() | ||
| + " for Webapp " + webapp.getMetadata().getName() + " in namespace " | ||
| + webapp.getMetadata().getNamespace()); | ||
| } | ||
|
|
||
| Tomcat tomcat = tomcatValue.get(); |
There was a problem hiding this comment.
This makes the usage a little bit more complex, though I guess it forces users to handle the null case. This is something I often wonder about actually when designing an API: should we use Optional in similar cases or follow a Map-like convention of returning null if the element is not present?
There was a problem hiding this comment.
IMHO, map like convention was there before Optional was present in Java. It make sense and happens in multiple examples that the secondary resources is checked for null. It should be, since it's not necessary present, if not created yet for example. For that case it's better to use Optional I think. Exactly as you say, that it forces users to check, also to eliminate null checks
metacosm
force-pushed
the
secondary-resource-optional
branch
from
601aa5d to
cc32576
Compare
This PR should be reviewed first:
#775