ok2c commented on code in PR #870:
URL:
https://github.com/apache/httpcomponents-client/pull/870#discussion_r3783229567
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java:
##########
Review Comment:
@arturobernalg Do not you agree this bit of code should be refactored /
fixed?
```
if (request.getPath().contains("?")) {
if (neverCache1_0ResponsesWithQueryString &&
from1_0Origin(response)) {
LOG.debug("Response is not cacheable as it had a query
string");
return false;
} else if (!neverCache1_1ResponsesWithQueryString &&
!isExplicitlyCacheable(cacheControl, response)) {
LOG.debug("Response is not cacheable as it is missing
explicit caching headers");
return false;
}
}
```
Should not it be something like this, so that in normal execution flow those
bits would never get executed?
```
if (neverCache1_0ResponsesWithQueryString &&
request.getPath().contains("?") &&
from1_0Origin(response)) {
LOG.debug("Response is not cacheable as it had a query string");
return false;
}
if (neverCache1_1ResponsesWithQueryString &&
request.getPath().contains("?") &&
!isExplicitlyCacheable(cacheControl, response)) {
LOG.debug("Response is not cacheable as it is missing explicit
caching headers");
return false;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
