[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 11/13/2013 08:04 AM, Hank Grabowski wrote:
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

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.

+1 for the separate EventHandler interface. I like the upgrade path as well, it seems similar to the pattern used by the Thread and Runnable classes, which developers should be familiar with.

Best Regards,

In the mean time I can make the above suggested changes and continue on with the refactoring of the test cases to reflect the new unified elevation detector code. 


        final EventDetector rawEvent = new ElevationDetector(maxcheck,
threshold, handler, sta1Frame).withConstantValue(elevation);

Using this methodology I was able to get all the unit tests passing with
the new unified event handler.  For tests that have explicit subclasses I
used the standard method.  Only in cases where the tests were written with
anonymous subclasses did I use the handler method.  I still have to
refactor the ElevationDetector JUnit test to pull in the test cases that
were in ApparentElevation and GroundElevationMask detector test harnesses.

The last step is supposed to be attaching the file(s) to the incident for
submission.  I will do that once the test has been refactored, hopefully

Thanks  lot for this contribution.