[Orekit Developers] Re: new sp3 parser pacakge

[Thomas Neidhart <thomas.neidhart@spaceapplications.com>]

On 12/19/2011 09:30 AM, MAISONOBE Luc wrote:
> Hi Thomas,
> 
> I have seen you committed your sp3 parser package. This is great, thanks
> a lot.
> 
> I quickly reviewed it, here are my comment. As you will see, they are
> minor. I think it is best to discuss all technical aspects on the dev
> list, so all the community can join the discussion.

sure, I was not fully aware of this mailinglist, but I am now
subscribed. Thanks for the comments, I was aware that there might be
some areas that require discussion:

> - in the SP3Parser.parseHeaderLine method, the scanner should be configured
>   with scanner.useLocale(Locale.US), otherwise it uses system defaults and
>   may fail to parse double numbers in some configuration (at least it fails
>   on my French configured Linux computer)

ok, good to know that, I did not think about that, will be fixed.

> - in the SP3ParserTest, the junit.framework.Assert import should be
> replaced with org.junit.Assert

ack.

> - in the SP3ParserTest, there are some spurious import statements (or the
>   position/velocity check should be added, I think they will use these
> imports)

yes, they are from the PV checks, which will be added.

> - the inner enumerate OrbitType in SP3File may be confused with the
> enumerate with the same name in the orbits package, do you think we should
> change its name ?

yes, I have seen this too and you are right, this is confusing. I would
propose OrbitFileType then.

> - I'm not sure whether OrbitFile, OrbitFileParser, SatelliteInformation and
>   SatelliteTimeCoordinate should be in the file package or in the file.sp3
>   package. Are they sp3 specific ? As the file package will later be
> populated
>   with other standards (like CCSDS), I don't really know where they
> should be

The idea was to have some kind of generic orbit file / parser types, but
as long as there is only a SP3 file parser it does not make much sense I
guess. For now we can move them to the sp3 package or see the
proposition below.

> - as you note in the files, some specific errors should be created

ack.

> - when everything will be ready, we should add a specific entry in the apt
>   documentation, and a bullet in the features list (which is in two places,
>   once in the index.apt file and once in the top level overview.html file
>   from the source tree javadoc)

ack.

> - you should now put your name in the developers section of the pom file
> ;-) (note that we have chosen to not put any email addresses there)

ack.

> - the header does not match the regular expressions, it misses a copyright
>   line, of course the copyright here should be yours, not CS

ok, I was not sure about that, but will add a proper copyright.

> - there are a bunch of checktsyle warnings (well there are also many
> warnings

I went through all of the warnings (using the checkstyle plugin for
eclipse) and hoped that it will be ok like that.

Most of the remaining warnings are related to the spurious:

is not designed for extension - needs to be abstract, final or empty

which I do not fully understand to be honest, and magic number warnings
for the parser, which is understandable considering the way it has been
implemented.

For the maximum length of a line: following the experience I got from
commons-math, I manually formatted the code and at some places where it
would be too long (e.g. 82 chars) but better to read, I kept it like
that. Is this the way you want also want it for orekit?

> - there are a few findbugs errors  (well there are also errors from other
>   files we changed recently, so we should also fix our own stuff ...)

ack, I did not yet run findbugs on it, but will do so.

> Most of these comments are indeed easy to fix details. What is to be
> decided together is where we put the top files. What do you think ?

Do you mean the OrbitFile and OrbitFileParser classes? Or the package
files itself? Something that came to my mind is to create a package
structure like this:

 org.orekit.files
              |
              >--orbit
                   |
                   >---sp3
                   >---rinex
                   >---...

Cheers,

Thomas