Provide a public factory for BiTransportObserver
#1135
Conversation
Motivation: Users who need to report events to two `TransportObserver`s simultaneously on the server side or who need their own variant of `TransportObserverConnectionFactoryFilter` will find it useful. Modifications: - Move `BiTransportObserver` from `client-api` to `transport-api`; - Convert passes first and second observers into safe observers; - Add public `TransportObservers.biTransportObserver` factory; Result: Users can easily report events to two `TransportObserver`s.
| * @return a {@link TransportObserver} that delegates all invocations to the {@code first} and {@code second} | ||
| * {@link TransportObserver}s | ||
| */ | ||
| public static TransportObserver biTransportObserver(final TransportObserver first, final TransportObserver second) { |
There was a problem hiding this comment.
For a public API, I would prefer a combine() method with a var-arg and if we want, we can optimize for 2 vs many cases.
There was a problem hiding this comment.
Good idea! Added support for multiple
| */ | ||
| public static TransportObserver biTransportObserver(final TransportObserver first, final TransportObserver second) { | ||
| return new BiTransportObserver(first, second); | ||
| public static TransportObserver combine(final TransportObserver first, final TransportObserver second, |
There was a problem hiding this comment.
Why not just have combine(final TransportObserver... observers) ?
Also, may be modify the BiTransportObserver to accept an array instead of two observers.
There was a problem hiding this comment.
At least two observer are required to combine. Good to highlight that in the API instead of verifying at runtime.
Having an array inside BiTransportObserver will require an array inside every observer level, producing boilerplate at every level to traverse the array. Let's start with simplified approach and we can reconsider if necessary.
| public static TransportObserver combine(final TransportObserver... other) { | ||
| switch (other.length) { | ||
| case 0: | ||
| throw new IllegalArgumentException("At least one TransportObserver is required to combine"); |
There was a problem hiding this comment.
This is what I was trying to avoid by having first, second, others arguments: http://jtechies.blogspot.com/2012/07/item-42-use-varargs-judiciously.html
There was a problem hiding this comment.
I think this is OK and inline with what we do elsewhere:
| public static TransportObserver combine(final TransportObserver... other) { | ||
| switch (other.length) { | ||
| case 0: | ||
| throw new IllegalArgumentException("At least one TransportObserver is required to combine"); |
There was a problem hiding this comment.
I think this is OK and inline with what we do elsewhere:
Motivation: Users who need to report events to two `TransportObserver`s simultaneously on the server-side or who need their own variant of `TransportObserverConnectionFactoryFilter` will find it useful. Modifications: - Move `BiTransportObserver` from `client-api` to `transport-api`; - Convert passes first and second observers into safe observers; - Add public `TransportObservers.biTransportObserver` factory; Result: Users can easily report events to two `TransportObserver`s.
Motivation: Users who need to report events to two `TransportObserver`s simultaneously on the server-side or who need their own variant of `TransportObserverConnectionFactoryFilter` will find it useful. Modifications: - Move `BiTransportObserver` from `client-api` to `transport-api`; - Convert passes first and second observers into safe observers; - Add public `TransportObservers.biTransportObserver` factory; Result: Users can easily report events to two `TransportObserver`s.
Motivation:
Users who need to report events to two
TransportObservers simultaneouslyon the server-side or who need their own variant of
TransportObserverConnectionFactoryFilterwill find it useful.Modifications:
BiTransportObserverfromclient-apitotransport-api;TransportObservers.biTransportObserverfactory;Result:
Users can easily report events to two
TransportObservers.