On 06/03/2011 09:22 PM, Bill Burke wrote:
> Some initial things. I'll fork on github again with my suggested changes to give you more precise feedback.
>
> - I don't get the point of having Client be an abstract class. Expecially since all non-static methods except close()
> and finalize are abstract already. I suggest making ClientFactory an abstract class and making Client an interface.
I don't quite understand why you push so hard for an interface in this particular case. Can you please provide more
explanation?
I made the client an abstract class to:
1. avoid the need for a separate factory class. To me, factory class in this case is just an artificial holder of
factory methods.
2. implement the instance disposal properly.
>
> - I still don't get the point of the HttpRequest.builder() I know what you're trying to do there as per previous email
> threads, but consider this case:
>
> I think that you'll still want to the ability to change the request's method within an interceptor. For example, there
> may be various services that tunnel DELETE and/or POST requests through a header or query parameter. You may want to
> abstract this away from the client so that client code is still using a delete() call, but an interceptor is changing it
> to a POST with a query parameter.
There is a HttpRequest.method(String name) designed to cover such use cases. My primary concern was to avoid incomplete
HttpRequest instances (either no URI or no HTTP method set). The proposed builder pattern provides a way how to achieve
this without a need to define a large set of overloaded factory methods on the Client for different URI input parameter
types, e.g.:
prepareGet(URI uri);
prepareGet(String uri);
prepareGet(UriBuilder builder);
preparePost(URI uri)
...
>
> - Why does this method return a future?
>
> <T> Future<T> start(InvocationCallback<T> callback);
>
> To make it cancelable?
Yes. And generally to be able to control or check the status of the request invocation.
>
> - You don't need the InvocationCallback's getType() and getGenericType() methods. The generic information is available
> from the getClass() method of the callback instance object.
>
> class MyCallback implements InvocationCallback<List<String>>
> {
> ...
> }
>
> Type type = callbackObject.getClass().getGenericInterfaces[0];
Done.
>
> - InvocationCallback's Javadoc should specify that the generic type can be a HttpResponse or entity type.
Ok, added. Thanks.
>
> - I don't like all the getFeature() methods. Just have a getProperties() method and be done with it.
It's a convenience method. The experience from Jersey is that many properties actually express features which are
bivalent in their nature and as such often modeled as boolean properties. I thought it makes sense to make clear
distinction between general configuration of the client and the requested features. The proposed convenience methods
seem to be a good solution IMO.
Marek