Re: [Orekit Developers] Unified Elevation Pattern Progress and Dilemma

My days of using usenet/mailing lists regularly are way in the past, so this is my first attempt at doing this through gmail. Hopefully I don't bork up inline comments...

On Wed, Nov 13, 2013 at 7:54 AM, MAISONOBE Luc <luc.maisonobe@c-s.fr> wrote:

Hank Grabowski <hank@applieddefense.com> a écrit :

This methodology works great for explicit subclasses but I don't think
there is a way to get it to work with anonymous subclasses. Anonymous
classes by definition don't have constructors. Since the create method is
really wrapping a constructor in a way that will allow the subclass to
inject its own constructor into the creation calls from the builder
methods, there is no real way to get the anonymous class to override
create. Without that behavior we can get the initial object of the
anonymous class type, but once we chain together a bunch of builder method
calls we are going to end up with a MyBaseClass not the anonymous
MySubClass.

You are right, anonymous subclassing does not work with this pattern. I don't think this is a real showstopper as it is always possible to replace anonymous subclassing by named subclassing, even using a small internal class if needed.

I guess the only thing is that for many of the cases we've used this in the past we've really liked being able to compress the code by doing anonymous subclassing for cases where we are simply changing logging or continue behavior. I'm not sure if we are in the norm or an anomaly.

Apart from the fluent/builder pattern above, I think the interface is really a good thing and worth introducing it on its own. I would simply make sure the T parameter is restricted to T extends EventDetector. It would really be worth having this for all events detectors (and better yet even for Apache Commons Math too) and move the eventOccurred method out of the detector interface. I was never happy with the default implementation of eventOccurred that mostly forced subclassing but without being explicitly abstract. It was a design error I made, sorry for this.

I agree with those modifications

Unfortunately, completely removing eventOccurred from this interface cannot be done for 6.1, it will have to wait for 7.0 as it is a large change on an important public interface users must implement in their application code. So what can we do in the interim? We could probably implement it at the AbstractDetector level first, so it is available for all Orekit predefined events, using a withEventOccurredHandler builder method too. current code would work as it does now, both in case the eventOccurred method is overriden or not in user code. If user code is adapted to call withEventOccurredHandler, then the default implementation of eventOccurred would delegate to it, and it will work well. It seems a good transition path.

I agree this sounds like a good approach as well. A benefit of this is that we could create a collection of standard generic handlers for cases where the caller object (I'm not sure I'm thrilled with that name, maybe "detector" would be better) isn't relevant. As you said though, this should wait for 7.0.