I started working with the Orekit 14 snapshot a bit more and found a few oddities when updating some existing code to use it. I thought I’d post here since 14 is a major release and it would be nice to get the API right. I decided to ask first in case someone has already thought of these and there is a good reason they are the way they are. I can help make the changes.
PVC Class Hierarchy
AbsolutePVCoordinates and PVCoordinates both have methods to getPosition(), getVelocity() etc., but they share no super type, so I can’t declare a variable that can accepts both a PVCoordinates and an AbsolutePVCoordinates. To add another oddity, TimeStampedPVCoordinates extends PVCoordinates while AbsolutePVCoordinates does not. In Orekit 13 AbsolutePVCoordinates extended TimeStampedPVCoordinates, so if I just want the position and velocity, I could declare a PVCoordinates and it would accept all three types. As an example use case, in Orekit 14 I can no longer use OrekitMatchers.pvIs(pv) with an AbsolutePVCoordinates.
Proposed fix is to either introduce an interface that all three classes can implement to get position & velocity, or make AbsolutePVCoordinates extend TimeStampedPVCoordinates like it did in Orekit 13.
PropagatorBuilder and OrbitalStateFactory
I see bits like this in the Orekit 14 code:
public KeplerianPropagatorBuilder(final OrbitalStateFactory<? extends Orbit> factory, ...) {
super((AbstractOrbitFactory<Orbit>) factory, ...);
Usually casing a class or using a cast to erasure generics is a clear symptom of weak API design. Here the constructor asks for an instance of an interface (good, user can provide their own implementation), but then casts to a specific subclass of that interface (bad, user actually can’t provide their implementation, and the compiler can’t check it). Casting ? extends Orbit to Orbit causes potential heap pollution (unexpected ClassCastException at run time) and the user code won’t see the compiler warning.
Another consturctor makes a similar mistake:
public NumericalPropagatorBuilder(final AbstractOrbitFactory<? extends Orbit> factory, ...) {
super((AbstractOrbitFactory<Orbit>) factory, ...);
Where the signature includes asking for an abstract class instead of the interface. This prevents the user from providing their own implementation, largely counteracting the advantage of defining an interface.
I propose asking for instances of a interface instead of an abstract class in methods and constructors, pulling up any needed methods from the abstract class to the interface, and using generics correctly so that the compiler can perform type checking.
Clone
Several classes continue using clone() . Using clone() introduces some oddities that cannot be reproduced using normal Java, making harder for developers to reason about the code. See Joshua Bloch’s chapter on it in “Effective Java”. Orekit 14 would be a good time to switch to using a copy constructor, or a copy() method, or both. That way no unusual “clone” semantics are introduced. And some of the clone implementations are already trying to be a copy() method. For example, this implementation where the call to super.clone() is only to trick static analysis tools:
public DSSTPropagatorBuilder clone() {
// Call to super clone() method to avoid warning
final DSSTPropagatorBuilder clonedBuilder = (DSSTPropagatorBuilder) super.clone();
EOPEntry and EOPOrigin
How do I specify that the EOPEntry was built from a source that didn’t contain nutation data? (I sometimes load a mark3.out file to match legacy applications.) I see the EOPEntry constructor replaces null:
this.nutOrigin = nutOrigin == null ? dtOrigin : nutOrigin;
this.cipOrigin = cipOrigin == null ? dtOrigin : cipOrigin;
Maybe allow null, or create a special value for “data missing”.
TLE Perigee
Very minor comment about naming, so I hesitate to bring it up. I saw TLE.getPerigeeArgument() was renamed to TLE.getPeriapsisArgument(). Perigee seemed like accurate nomenclature for a TLES. Maybe the change was just part of standardizing nomenclature across Orekit.
Best Regards,
Evan