OAuthFetcher constructor params in r678354

[Brian Eaton <be...@google.com>]

etnu wrote:
> Now that we're up to 7 parameters on this constructor, can we accept that
> making everything an HttpFetcher is not the best design choice we've ever
> made? :) This is especially bad since only two of these parameters vary per
> invocation. This is extremely confusing code to follow, and could be much
> better written as a singleton with an inner class for request handling.

... which would move the code from using an OAuthFetcherFactory and an
OAuthFetcher to using OAuthFetcher and an inner class.  You still have
two classes, and the two classes need to implement the exact same
logic.  It doesn't make things simpler, it just moves complexity.

About the number of parameters that will vary: nextFetcher, params,
and authToken are all reasonable things to vary per-request.  The rest
are there for dependency injection.  We could use Guice for them, I
suspect.

Re: OAuthFetcher constructor params in r678354

[Brian Eaton <be...@google.com>]

OK.  A good first step might be to pass in a reference to the
OAuthFetcherFactory instead of individual parameters.  Or the factory
could create an OAuthOptions object that contains the necessary.

On Mon, Jul 28, 2008 at 1:47 PM, Kevin Brown <et...@google.com> wrote:
> On Mon, Jul 28, 2008 at 10:44 AM, Brian Eaton <be...@google.com> wrote:
>>
>> etnu wrote:
>> > Now that we're up to 7 parameters on this constructor, can we accept
>> > that
>> > making everything an HttpFetcher is not the best design choice we've
>> > ever
>> > made? :) This is especially bad since only two of these parameters vary
>> > per
>> > invocation. This is extremely confusing code to follow, and could be
>> > much
>> > better written as a singleton with an inner class for request handling.
>>
>> ... which would move the code from using an OAuthFetcherFactory and an
>> OAuthFetcher to using OAuthFetcher and an inner class.  You still have
>> two classes, and the two classes need to implement the exact same
>> logic.  It doesn't make things simpler, it just moves complexity.
>>
>> About the number of parameters that will vary: nextFetcher, params,
>> and authToken are all reasonable things to vary per-request.  The rest
>> are there for dependency injection.  We could use Guice for them, I
>> suspect.
>
> Yes, exactly -- the outer class can use method injection (or a builder
> perhaps) for these and can have the inner objects have references to them.
>
> When there's 7 parameters, it's very hard to follow at the call site. I'd
> argue against anything with more than 3 or 4 parameters. Builders and
> setters are much easier to follow.
>
>

Re: OAuthFetcher constructor params in r678354

[Kevin Brown <et...@google.com>]

On Mon, Jul 28, 2008 at 10:44 AM, Brian Eaton <be...@google.com> wrote:

> etnu wrote:
> > Now that we're up to 7 parameters on this constructor, can we accept that
> > making everything an HttpFetcher is not the best design choice we've ever
> > made? :) This is especially bad since only two of these parameters vary
> per
> > invocation. This is extremely confusing code to follow, and could be much
> > better written as a singleton with an inner class for request handling.
>
> ... which would move the code from using an OAuthFetcherFactory and an
> OAuthFetcher to using OAuthFetcher and an inner class.  You still have
> two classes, and the two classes need to implement the exact same
> logic.  It doesn't make things simpler, it just moves complexity.
>
> About the number of parameters that will vary: nextFetcher, params,
> and authToken are all reasonable things to vary per-request.  The rest
> are there for dependency injection.  We could use Guice for them, I
> suspect.


Yes, exactly -- the outer class can use method injection (or a builder
perhaps) for these and can have the inner objects have references to them.

When there's 7 parameters, it's very hard to follow at the call site. I'd
argue against anything with more than 3 or 4 parameters. Builders and
setters are much easier to follow.