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

add optional monad from java8 #70

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@acoburn
Copy link
Contributor

commented May 19, 2015

See: https://jira.duraspace.org/browse/FCREPO-1550

This is a non-functional change, representing an effort to get my feet wet with Java8. I would appreciate it if @ajs6f could look over the code to make suggestions where the expressions are not idiomatic.

@@ -81,7 +82,7 @@ public FcrepoResponse head(final URI url)
final HttpRequestBase request = HttpMethods.HEAD.createRequest(url);
final HttpResponse response = executeRequest(request);
final int status = response.getStatusLine().getStatusCode();
final String contentType = getContentTypeHeader(response);
final Optional<String> maybeContentType = getContentTypeHeader(response);

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

This (and all that follows on it below with the use of Optional::ifPresent) isn't unarguably great. See Stuart Marks' explanation. Long story short: you're shortening code but trading an explicit nullcheck for an object creation and an implicit nullcheck. I don't have a problem with it, but I know some people wouldn't be enthused.

This comment has been minimized.

Copy link
@acoburn

acoburn May 20, 2015

Author Contributor

@ajs6f this is precisely the feedback I'm looking for. It's a bit unclear to me when Optional should be used, if at all in this code. That is, I'm not at all attached to the refactoring in this PR -- it is more an exercise in getting to know the Optional class.

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

Well, so this is the thing. Here're some thoughts:

Some of the gains in brevity in the main code are lost by expanded test code. You're eventually losing a few dozen lines over a few hundred lines, so the real question is: is it clearer? That's a matter of taste. See below for further comment.

This comment has been minimized.

Copy link
@acoburn

acoburn May 20, 2015

Author Contributor

I am going to close this PR and view it as a practice exercise. I agree that it doesn't seem to add much in clarity.

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

I'd be looking harder at Stream. There's a lot of quick wins in that one.

final String url = getUrl(exchange, transaction);
final String prefer = getPrefer(exchange);
final String contentType = Optional.ofNullable(endpoint.getContentType())
.filter(x -> x.length() > 0)

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

Just Guava's Strings::isNullOrEmpty.

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

Actually, maybe better StringUtils::isNotEmpty to avoid the negation.

@@ -46,7 +47,7 @@
* @param body the response body stream
*/
public FcrepoResponse(final URI url, final int statusCode,
final String contentType, final URI location, final InputStream body) {
final Optional<String> contentType, final Optional<URI> location, final InputStream body) {
this.setUrl(url);

This comment has been minimized.

Copy link
@acoburn

acoburn May 20, 2015

Author Contributor

@ajs6f some questions on this change -- the FcrepoResponse class has a number of String/URI fields, two of which were changed to Optional<T>. Is this completely unnecessary (as per the comment above)? Should they be left as they were, with null checks in the implementation code?

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

You could do some statistics... Determine how many of the places you are now using Optional methods could be substituted with the ternary operator. That's just as brief and clear and runs faster. Every time you see the use of Optional solely to avoid nullchecks that choose between a possible value and a default, you could use the ternary operator instead. The only place you're really getting briefer code is if there is some action to be taken if the value is available and there is nothing to be done (no suitable default or calculation that can supply a default) without the value. That's going to move you from an if () {} to Optional::ifPresent. Keep in mind that if () (no braces, one line) is almost as crisp as Optional idioms, but I think that @awoods' choice of Checkstyle configs has made it unavailable. We should probably fix that.

Some of this is just a question of style, but I'm trying myself to take Marks' advice to heart. No one knows more about this thing than does he, and if he says that an Optional-typed field is a misuse of the API, that worries me because we might find ourselves ill-served in future by having so done.

@@ -198,7 +175,8 @@ private String getContentType(final Exchange exchange) {
private String getAccept(final Exchange exchange) {
final Message in = exchange.getIn();
final String fcrepoTransform = in.getHeader(FcrepoHeaders.FCREPO_TRANSFORM, String.class);
final String acceptHeader = getAcceptHeader(exchange);
final String acceptHeader = Optional.ofNullable(in.getHeader(Exchange.ACCEPT_CONTENT_TYPE, String.class))
.orElse(in.getHeader("Accept", String.class));

if (!isBlank(endpoint.getTransform()) || !isBlank(fcrepoTransform)) {

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 20, 2015

Contributor

You could use Stream::of here with the values to check and then Stream::findFirst to get the first non-blank choice, with Optional::orElse for the case of DEFAULT_CONTENT_TYPE.

@acoburn acoburn closed this May 20, 2015

@acoburn acoburn deleted the acoburn:java8 branch Jul 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.