Hi folks,
I have made a shallow, non-exhaustive (maybe wrong) review of the
codebase before 5.0 GA to have the chance to improve things which will
be frozen afterwards:
Specific spots:
* org.apache.hc.core5.net.Host.create(String) +
org.apache.hc.core5.net.URIAuthority.create(String) +
org.apache.hc.core5.http.HttpHost.create(String):
** Will fail on IPv6 addresses when the port is extacted. One has to
probe for brackets: []
Note that those brackets likely need to be dropped when a socket is
obtained, but readded when
a string compound 'host:post' is build
* org.apache.hc.core5.net.InetAddressUtils
** Parsing may fail when an IPv6 scope id might be provided
* org.apache.hc.core5.reactor.InternalDataChannel.startTls(SSLContext,
NamedEndpoint, SSLBufferMode, SSLSessionInitializer, SSLSessionVerifier,
Timeout):
** Eclipse tells me: resource leak: '<unassigned Closeable value>' is
never closed
If that is not the case maybe a comment or a suppress should be added
*
org.apache.hc.core5.reactor.IOReactorConfig.Builder.DefaultMaxIoThreadCount:
** Why is that pascal case?
* org.apache.hc.core5.reactor.IOReactorConfig.toString():
** selectIntervalMillis: TimeValue already provides a unit: should be
'selectInterval'
* org.apache.hc.core5.reactor.MultiCoreIOReactor.toString():
** Is it really helpful to call super.toString() here?
* org.apache.hc.core5.http.impl.bootstrap.HttpServer.HttpServer(int,
HttpService, InetAddress, SocketConfig, ServerSocketFactory,
HttpConnectionFactory<? extends DefaultBHttpServerConnection>,
Callback<SSLParameters>, ExceptionListener)
** Partial bound check only for the port
** I am also confused why the port comes before the address in the
constructor
*
org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.updateInputMetrics(HttpResponse,
BasicHttpConnectionMetrics) and other spots:
** Use a constant for that literal
* org.apache.hc.core5.http.message.HeaderGroup.getHeader(String):
** The exception message looks odd
* org.apache.hc.core5.http.message.ParserCursor:
** Could probably use Args magic to avoid duplicate code
* org.apache.hc.core5.http.Chars
** I've seen several spots in the code redefining the same constants
over and over again
* org.apache.hc.core5.http.ContentType:
** I am confused why almost all 'application/*+xml' use ISO-8859-1
although default encoding of XML is UTF-8
* org.apache.hc.core5.util.Args.notNull(T, String):
** Don't throw IAE on null. Null requires an NPE. This is has been
concensus for many years. Also done so in Commons Lang's Validate class
* org.apache.hc.core5.util.Asserts:
** Why retain if there is Args?
* org.apache.hc.core5.util.LangUtils:
** Remove methods which are in Objects already
General:
* Using System.currentTimeMillis() is discouraged for measuring elapsed
time, one shall use System.nanoTime()
* This may be highly subjective, but when Args is used to check nulls or
similar, the same arg name style, e.g.,
requestTimeout shall be retained in the final string. It makes
grepping and identifying easier for the developer.
* On several occasions when no value is provided by the client a literal
default value is used, constants would be better in such cases.
* Inconsistent buffer sizes, some use 2048 other 8192. If on purpose
then it might require documentation.
* As discussed recently for the TimeValue class, this now applies in
general:
I highly dislike the usage of '%,d' as all texts are in English, but
the target locale is unknown.
This will cause confusion with non-English clients. It shall be
reverted/adjusted to %d.
* I don't know how far this code has been reviewed for RFC 7230 and
friends, but there are a lot of refs to the old RFCs
* URI/host parsing. I have seen code which is done over and over again,
e.g., Host and HttpHost. Isn't HttpHost simply a specialization of Host?
My deepest kudos to all committers who created this massive amount of
decent code!
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
