Am 2015-12-01 um 21:46 schrieb Oleg Kalnichevski:
On Tue, 2015-12-01 at 21:29 +0100, Michael Osipov wrote:
Am 2015-12-01 um 14:35 schrieb Oleg Kalnichevski:
Folks
I would like to start cutting RC1 for HttpCore 5.0 alpha1 sometime soon.
Could you please take a look at the latest snapshot and let me know if
you find anything that might block the release or cause another RC
build?
While this is an alpha release, the code does not show overall good
fitness. It will take a look of tweaks for GA.
I've made a quick code review and here are my findings:
1. What about the artifact ids? Didn't we want to apply the same scheme
as Apache Commons?
httpcore5-parent
|- httpcore5
|- httpcore5-ab
etc..
2. httpcore/src/main/appended-resources/META-INF/...
This can be autogenerated with
https://maven.apache.org/apache-resource-bundles/
3. version.properties requires a relocation and an update
4. Remove all not used Subversion properties like Id, Author, etc. but
not eol-style of course.
5. How far are the imported annotations used throughout the code? Are
the annotated classed analyzed at runtime or do they serve mere
documentation which Javadoc could do?
6. Rename "io" packages to "bio" to make distinction from "nio" (uniform
naming)
7. Several comments still refer to the old RFC. Javadoc need to be
proof-read and updated to the new RFCs.
8. HttpResponse throws IllegalStageException for illegal arguments. Weird?!
9. Buffer sizes vary from 2 KiB to 8 KiB throughout the code. What about
normalization?
10. ContentType: character encoding of application/*-xml is set to
ISO-8859-1 but the default encoding for XML is UTF-8.
(I am ignoring the XML prolog for now)
11. HeaderGroup: Using a get(int), set(int, Object) on a list (here
List<Header>) might incur a performance penalty because they were not
designed for that.
12. TokenParser duplicates final statics from Chars
13. Several methods throughout the code declare throws
DerivedFromRuntimeException" this is code smell and needs to be removed.
Unchecked exception appear only in Javadocs.
14. HttpDateGenerator is marked as @ThreadSafe but SimpleDateFormat is
not. WTF?
15. Several wellknown literals are used where statics would be
appropriate like 80, 443, HTTP methods, "http", "https" etc.
16. System.currentTimeMillis() is used at several spots where elapsed
time is measured. This approach is obsolete/error-prone and shall
replaced with System.nanoTime()
17. ByteBufferAllocator and its implementors seem to be pretty much
useless/overdesigned just to wrap one method: allocate/allocateDirect,
aren't they?
18. Replace Args, Asserts, TextUtils, LangUtils, SimpleDateFormat (#14)
with Commons Lang. There is so much duplicate code that it is worth
having Commons Lang 3 as dependency.
19. What I absolutely do not like/understand is the inconsistent
approach of custom exceptions (not a flame war here please):
* At several points default exceptions are used inconsistently or
do not even make sense
* There is no clear distinction between recoverable and
unrecoverable exceptions, e.g.,
* why do I have catch a ParseException where I cannot do anything
about it?
* I cannot retry anyway when there is malformed input.
* Server sends an HTTP message not conforming with the RFCs, I cannot
do anything about it in my code either.
The exceptions require a major overhaul in this new major version.
I would work on them piece by piece if no one objects. If I need some
guidance, I will ask separately on the list. Feel free to comment on
those but please delete the unnecessary parts of this mail.
I _very_ much object to 13, 14, 16 to 19, _especially_ 18 and 19. I am
not in favor of point 6.
Can you please elaborate on your objection?
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
