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

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

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


I've elevated this to the message board as requested since the e-mail chain
was more than a couple of responses long.  To get everyone up to speed, we
want to have a unified elevation detector that combines masks, constant
elevation minimums and the ability to specify a refraction model (or none).
 This is for issue 144.

The problem we have run into is that the builder pattern has some
difficulties for subclassing.  The builder pattern is basically:

public class MyBaseClass {
   public MyBaseClass() {...}
   private MyBaseClass(args...) {...}

   ...

   public MyBaseClass withSomeSetting(...) {...}
   public MyBaseClass withSomeOtherSetting(...) {...}

}

where the "with" methods call the private constructor.  The problem arises
that we need to inherit from the base class in the case of eventing so that
we can override the event handler behavior.  The solution that was proposed
in the e-mail exchanges is to do the following:

public abstract class MyBaseClass {
  public MyBaseClass() {...}
  protected MyBaseClass(args...) {...}

  protected MyBaseClass create(args...) {...}

  public MyBaseClass withSomeSetting(...) {...}
  public MyBaseClass withSomeOtherSetting(...){...}
}

Then when you create the subclass you get:

public class MySubClass {
  public MySubClass() {...}
  private MySubClass(args...) {...}

  protected MyBaseClass create(args...) {...}

}
For those who wish want to understand the rational behind this pattern, they may want to read two threads about a similar feature in Apache Commons Math. The initial discussion about the concept is here: <http://markmail.org/thread/3fjvucyz7rax4cyi>. Further discussion about inheritance is here: <http://markmail.org/thread/nilg5ife36ujjawp>. 



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. 



If you guys have an idea on how to get around that strictly using the above
method, please post here and I can incorporate that instead, if you prefer

In the interim I have implemented a new interface called
EventOccurredHandler<T> which has one method: eventOccurred(SpacecraftState
s, T caller, boolean increasing).  This is nearly identical to the
eventOccurred in the AbstractDetector but allows for the injection of the
calling class.   That works out practically to be something like:

        final EventOccurredHandler<ElevationDetector> handler = new
EventOccurredHandler<ElevationDetector>() {
            private static final long serialVersionUID = 1L;
            public Action eventOccurred(final SpacecraftState s,
ElevationDetector caller, final boolean increasing) {
                    return Action.CONTINUE;
                }
        };
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.
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. 



        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
tomorrow.


Thanks  lot for this contribution.

Luc



--
Hank Grabowski
Chief Technology Officer

10440 Little Patuxent Parkway, Suite 600

Columbia, MD 21044

(410) 715-0005 Office

(410) 715-0008 Fax

(301) 525-6219 Mobile

hank@applieddefense.com

www.AppliedDefense.com <http://www.applieddefense.com/>





----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.