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.

Sign up for GitHub

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

Jump to bottom

add optional monad from java8 #70

Closed
wants to merge 1 commit into from
Closed

add optional monad from java8 #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
<!-- plugins -->
<build.helper.plugin.version>1.9.1</build.helper.plugin.version>
<bundle.plugin.version>2.5.3</bundle.plugin.version>
<checkstyle.plugin.version>2.14</checkstyle.plugin.version>
<checkstyle.plugin.version>2.15</checkstyle.plugin.version>
<compiler.plugin.version>3.2</compiler.plugin.version>
<dependency.plugin.version>2.10</dependency.plugin.version>
<doxia.plugin.version>1.3</doxia.plugin.version>
Expand Down
61 changes: 18 additions & 43 deletions src/main/java/org/fcrepo/camel/FcrepoClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.net.URI;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import org.apache.http.Header;
import org.apache.http.HttpEntity;
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


LOGGER.debug("Fcrepo HEAD request returned status [{}]", status);

Expand All @@ -91,7 +92,7 @@ public FcrepoResponse head(final URI url)
if (links.size() == 1) {
describedBy = links.get(0);
}
return new FcrepoResponse(url, status, contentType, describedBy, null);
return new FcrepoResponse(url, status, maybeContentType, Optional.ofNullable(describedBy), null);
} else {
throw new FcrepoOperationFailedException(url, status,
response.getStatusLine().getReasonPhrase());
Expand All @@ -112,12 +113,8 @@ public FcrepoResponse put(final URI url, final InputStream body, final String co
final HttpMethods method = HttpMethods.PUT;
final HttpEntityEnclosingRequestBase request = (HttpEntityEnclosingRequestBase)method.createRequest(url);

if (contentType != null) {
request.addHeader(CONTENT_TYPE, contentType);
}
if (body != null) {
request.setEntity(new InputStreamEntity(body));
}
Optional.ofNullable(contentType).ifPresent(x -> request.addHeader(CONTENT_TYPE, x));
Optional.ofNullable(body).ifPresent(x -> request.setEntity(new InputStreamEntity(x)));

LOGGER.debug("Fcrepo PUT request headers: {}", request.getAllHeaders());

Expand Down Expand Up @@ -168,12 +165,8 @@ public FcrepoResponse post(final URI url, final InputStream body, final String c
final HttpMethods method = HttpMethods.POST;
final HttpEntityEnclosingRequestBase request = (HttpEntityEnclosingRequestBase)method.createRequest(url);

if (contentType != null) {
request.addHeader(CONTENT_TYPE, contentType);
}
if (body != null) {
request.setEntity(new InputStreamEntity(body));
}
Optional.ofNullable(contentType).ifPresent(x -> request.addHeader(CONTENT_TYPE, x));
Optional.ofNullable(body).ifPresent(x -> request.setEntity(new InputStreamEntity(x)));

LOGGER.debug("Fcrepo POST request headers: {}", request.getAllHeaders());

Expand All @@ -190,8 +183,7 @@ public FcrepoResponse post(final URI url, final InputStream body, final String c
* @return the repository response
* @throws FcrepoOperationFailedException when the underlying HTTP request results in an error
*/
public FcrepoResponse delete(final URI url)
throws FcrepoOperationFailedException {
public FcrepoResponse delete(final URI url) throws FcrepoOperationFailedException {

final HttpRequestBase request = HttpMethods.DELETE.createRequest(url);
final HttpResponse response = executeRequest(request);
Expand All @@ -214,19 +206,14 @@ public FcrepoResponse get(final URI url, final String accept, final String prefe

final HttpRequestBase request = HttpMethods.GET.createRequest(url);

if (accept != null) {
request.setHeader("Accept", accept);
}

if (prefer != null) {
request.setHeader("Prefer", prefer);
}
Optional.ofNullable(accept).ifPresent(x -> request.setHeader("Accept", x));
Optional.ofNullable(prefer).ifPresent(x -> request.setHeader("Prefer", x));

LOGGER.debug("Fcrepo GET request headers: {}", request.getAllHeaders());

final HttpResponse response = executeRequest(request);
final int status = response.getStatusLine().getStatusCode();
final String contentType = getContentTypeHeader(response);
final Optional<String> maybeContentType = getContentTypeHeader(response);

LOGGER.debug("Fcrepo GET request returned status [{}]", status);

Expand All @@ -236,7 +223,7 @@ public FcrepoResponse get(final URI url, final String accept, final String prefe
if (links.size() == 1) {
describedBy = links.get(0);
}
return new FcrepoResponse(url, status, contentType, describedBy,
return new FcrepoResponse(url, status, maybeContentType, Optional.ofNullable(describedBy),
getEntityContent(response));
} else {
throw new FcrepoOperationFailedException(url, status,
Expand All @@ -262,18 +249,16 @@ private HttpResponse executeRequest(final HttpRequestBase request) throws Fcrepo
private FcrepoResponse fcrepoGenericResponse(final URI url, final HttpResponse response,
final Boolean throwExceptionOnFailure) throws FcrepoOperationFailedException {
final int status = response.getStatusLine().getStatusCode();
final URI locationHeader = getLocationHeader(response);
final String contentTypeHeader = getContentTypeHeader(response);

if ((status >= HttpStatus.SC_OK && status < HttpStatus.SC_BAD_REQUEST) || !throwExceptionOnFailure) {
return new FcrepoResponse(url, status, contentTypeHeader, locationHeader, getEntityContent(response));
return new FcrepoResponse(url, status, getContentTypeHeader(response),
getLocationHeader(response), getEntityContent(response));
} else {
throw new FcrepoOperationFailedException(url, status,
response.getStatusLine().getReasonPhrase());
}
}


/**
* Extract the response body as an input stream
*/
Expand All @@ -294,25 +279,15 @@ private static InputStream getEntityContent(final HttpResponse response) {
/**
* Extract the location header value
*/
private static URI getLocationHeader(final HttpResponse response) {
final Header location = response.getFirstHeader(LOCATION);
if (location != null) {
return URI.create(location.getValue());
} else {
return null;
}
private static Optional<URI> getLocationHeader(final HttpResponse response) {
return Optional.ofNullable(response.getFirstHeader(LOCATION)).map(x -> URI.create(x.getValue()));
}

/**
* Extract the content-type header value
*/
private static String getContentTypeHeader(final HttpResponse response) {
final Header contentType = response.getFirstHeader(CONTENT_TYPE);
if (contentType != null) {
return contentType.getValue();
} else {
return null;
}
private static Optional<String> getContentTypeHeader(final HttpResponse response) {
return Optional.ofNullable(response.getFirstHeader(CONTENT_TYPE)).map(x -> x.getValue());
}

/**
Expand Down
Loading