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. Oleg --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
