Hi,
thanks a lot for the feedback!!!
We'll address all your suggestions.
http://java.net/jira/browse/GRIZZLY-1259
Thanks.
WBR,
Alexey.
On 04/20/2012 02:56 AM, gonfidentschal_at_gmail.com wrote:
> i'm a new user of grizzly, embedded for jaxws. here are my findings
> after the first steps.
>
> i was not able to achieve my goals without downloading the grizzly
> source code. in other libraries like spring i rarely have to dig into
> the code to understand what's going on.
> - the javadoc alone is, in the areas where i needed it, too sparse.
> - the method names and parameters alone were not clear enough to me
> - there were some surprises
>
> here are some examples:
>
> -----------------------------------------------------------------------
> ----------------
> ThreadPoolConfig config = ThreadPoolConfig.defaultConfig();
> ok so now i've got the default config. there is a method .copy() on it.
> should i call it?
> 1) there is no javadoc on the method.
> 2) the method name suggests that i get the default config and should
> make a copy unless i really want to modify the defaults. looking at the
> code revealed that my intuition was wrong.
> suggestion:
> 1) add javadoc
> 2) maybe rename method (set to deprecated and add new one):
> copyDefaultConfig(). at least the method isn't called
> getDefaultConfig(). but i usually like the verbNoun() concept.
>
> -----------------------------------------------------------------------
> ----------------
> ThreadPoolConfig is a builder
> it took me a moment to figure out that this class is the configuration
> itself as well as a builder. the class header currently has zero
> javadoc, i would mention this.
>
> -----------------------------------------------------------------------
> ----------------
> networkListener.getTransport().setWorkerThreadPoolConfig(config);
> apidoc says:
> Set the {_at_link ThreadPoolConfig} to be used by the worker thread
> pool.
> @param workerConfig worker thread pool configuration.
>
> looking at the code reveals:
> if (isStopped()) {
> this.workerPoolConfig = workerPoolConfig;
> }
>
> 1) the javadoc contains a lot of noise. when the setter is called
> setWorkerThreadPoolConfig() then the javadoc "@param workerConfig
> worker thread pool configuration" is obvious.
> 2) the javadoc doesn't say that the call is silently ignored when the
> server is started already.
> 3) i disagree with the behavior of ignoring a setter when the server is
> started, and would instead expect and document an
> IllegalStateException.
>
> -----------------------------------------------------------------------
> ----------------
> HttpServer: incomplete javadoc
> the class header itself has zero javadoc.
> the methods start and stop only say the obvious, but don't say that
> they silently ignore the call if it is in that state already.
> also, i'd be interested in how stop() behaves; does it at first disable
> new incoming requests, and let currently running ones finish
> gracefully? or is it an immediate stop and kills them? are there
> alternatives to this call?
>
> -----------------------------------------------------------------------
> ----------------
> ThreadPoolConfig again: useless javadoc
> many methods look like this one:
> /**
> * @return the queuelimit
> */
> public int getQueueLimit() {
> return queueLimit;
> }
> i would prefer to have the noise "@return the queuelimit" removed so
> that my eyes have less to process.
> this one should indeed have some documentation imo. the special value
> -1 is used, and not in one place is documented what it means. i was
> able to figure it out, but mentioning it would remove all doubt and
> also remind new developers of the special case when they make changes
> to the code.
>
> -----------------------------------------------------------------------
> ----------------
> null handling
> the current concept of null handling is to not check for it. as a
> result exceptions occur late, and the cause is harder to find.
> also, the user doesn't know if he's allowed to hand in null at all
> (maybe the concept is to never allow null?).
> i personally would prefer to have methods annotated using jetbrains'
> null annotations @Nullable and NotNull. they document
> what's allowed plus enforce it, throwing an IllegalArgumentException
> instantly instead of getting a NPE anywhere later on.
> same for getters where atm i have to guess if the returned value could,
> under some circumstances, be null. without the
> annotation/documentation i'm forced to check for it.
>
> end of rant, and thanks for the software. maybe one thing or two of
> what i believe would improve the api are considered.