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