On 8/23/11 12:33 PM, Marek Potociar wrote:
>>
>> I want you to do an exercise:
>>
>> 1. Count how many Builder interfaces you have
>> 2. Calculate the average of how many interfaces an interface implements
>> 3. Load this revision into a graphical class hierarchy browser and view all the criss-crossing lines.
>
> That's something which would provide the implementor's point of view.
Not really. One of the first things I look at is the Javadoc *as a user*.
> http://java.net/projects/jax-rs-spec/sources/git/content/src/examples/src/main/java/jaxrs/examples/client/BasicExamples.java
>
>>
>> Then maybe you'll see how complicated you've made this API in order to make it "pure". IMO, ditch this, revert to the
>> previous version, and let's move on from there.
>
> Again, I am not ignorant of the increased complexity in the class hierarchy. But, again, that's not the goal of a fluent
> API. The goal is to be easy to use, DSL-like and semantically consistent. What we had before was simpler, but at the
> expense of compromises in the actual goals. Class hierarchy complexity can be dealt with by providing usage examples.
> What we really should be focusing on are the method names to make the DSL part of the API natural.
>
What exact compromises did the previous revision make (especially with
my added additions)?
>>
>> I also think there are a lot of problems with it:
>>
>> * How do you set the Content-Type of a Target? What I'm getting at is, what's the point of having Target implement the
>> SyncInvoker interface?
>
> Content type SHOULD be included when entity is not empty, but it is not a mandatory header. You can include it via:
> client.target(...).type("text/plain").put(entity);
>
> Or you can just omit it (not recommended):
> client.target(...).put(entity); // content type defaults to "application/octet-stream"
>
> Other option would be to add an extra parameter for content-type into the invoker methods that consume entity (PUT, POST
> ...). But we discussed this with Santiago and we feel that the extra parameter would make the method signatures somewhat
> complex.
>
> Yet another option would be to provide some default content type resolution support for most common entity types.
>
All this just so that you don't have to invoke a request() method (which
is fluent-compliant BTW) on Target. IMO, which I've stated in the past,
a request() method makes a clear separation of concerns.
Target.header(foo,bar) makes as much sense as URL.header(foo, bar).
Target is a resource, not a request.
>>
>> * The only way to go from a Target to an Invocation/Invocation.Builder is to set a header on Target. What if you don't
>> want/need to set a header? i.e. a get or delete request?
>
> We can always add a new method to Target, but before we do it, I am interested in seeing a particular use case. Maybe it
> can be solved without the extra method.
>
I want to remove Target implementing SyncInvoker (and RequestHeaders,
etc.). Once you do that, you can pretty much eliminate most of the
additional abstractions you added to the latest revision.
>>
>> * Client.Builder and ClientFactoryBuilder and ClientFactory can be combined. There's no need to have a factory of a
>> factory of a factory. Please see my earlier proposal (4-5 weeks ago) on a better way to do this.
>
> ClientFactory is a common entry point. As such it has to be an abstract class.
> ClientBuilderFactory is the implementation provider hook.
> Client.Builder is the extensible fluent client bootstrapping base class.
>
> The way those classes currently work together let's you write code like this (notice the type-safe requestQueueCapacity
> config option):
>
> ClientFactory.newClientBy(ThrottledClient.Builder.Factory.class).requestQueueCapacity(10).build();
>
> This is a line from the BasicExamples.clientBootstrapping(). How would you support this type-safe bootstrapping
> configuration with the reduced API?
>
I think this suffers a little bit from over-engineering.
What is wrong with a simple constructor? You're already referencing the
vendor specific class.
ThrottledClient tc = new ThrottledClient();
tc.requestQueueCapacity(...)
.timeout(...)
.cacheSize(...)
Or even:
ThrottledClient tc =
ThrottledClient.create().requestCapacity(...).timeout().cacheSize();
Its not the specs domain to define how an implementor provides
implementation specific features/APIs.
>> The thing is, I doubt anybody will care if its 100% fluent. The
previous version was already pretty much 99% fluent.
>> Even that 1% difference was up for debate, IMO on whether it was
"fluent" or not.
>
> The problem was with the verb not being at the end. It sure had to
bother you too as you tried to address it in your
> proposal.
>
Yeah, and I solved the verb problem pretty simply, not by adding mounds
of abstractions.
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com