Great, Martin. Feel free to suggest the same below in Review Board where you can line by line do the comments below (and then in turn they will get emailed to the mailing list).
Cheers, Chris ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Chris Mattmann, Ph.D. Senior Computer Scientist NASA Jet Propulsion Laboratory Pasadena, CA 91109 USA Office: 171-266B, Mailstop: 171-246 Email: [email protected] WWW: http://sunset.usc.edu/~mattmann/ ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Adjunct Assistant Professor, Computer Science Department University of Southern California, Los Angeles, CA 90089 USA ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ -----Original Message----- From: Martin Desruisseaux <[email protected]> Organization: Geomatys Reply-To: "[email protected]" <[email protected]> Date: Tuesday, June 4, 2013 2:05 AM To: "[email protected]" <[email protected]> Subject: Re: Material for potential volunteer >Hello Travis > >Le 04/06/13 04:40, Travis L Pinney a écrit : >> Uploaded the patch here. >> >> https://reviews.apache.org/r/11615/ > >Thanks! Can I suggest a few updates? > >It would be nice to have a little bit of javadoc, if possible with links >to the Shapefile specification or wikipedia page related to the code. >For example in ShapeTypeEnum, we don't know what the 'value' integer is... > >On a minor note, lines like below: > > StringBuilder s = new StringBuilder(); > s.append("FileCode: " + this.FileCode + "\n"); > >Would be more efficient like below: > > StringBuilder s = new StringBuilder(); > s.append("FileCode: ").append(this.FileCode).append('\n'); > >You may also consider replacing '\n' by lineSeparator, where: > > String lineSeparator = System.getProperty("line.separator", "\n"); > >I noticed that many field name begins with an upper-case letter, while >the Java convention is to start with lower-case letter. Could it be >adjusted? > >In the following line: > > return new String(this.FieldName); > >The encoding need to be specified. I would expect the encoding to be >specified somewhere in the Shapefile specification. For example if the >encoding is ISO Latin 1, then the line should be: > > return new String(this.FieldName, "ISO-8859-1"); > >I would also suggest to omit the "this." prefix, unless there is a risk >of confusion with local variables. > > >I would also suggest to avoid using MappedByteBuffer, and use plain >ByteBuffer/ReadableChannel pair instead. The reason is that >MappedByteBuffer is potentially heavy, and is usually reserved for very >large file which are going to be open for a relatively long time. >MappedByteBuffer are especially useful when bytes are going to be read >in random order. For example I find MappedByteBuffer ideally suited for >index. > >However in this case, we are reading data sequentially. It would be nice >to avoid taking more OS resources than needed. Especially since the >current Shapefile code creates 2 MappedByteBuffer and I suspect that one >of them is for a small file. I realize that a ByteBuffer/ReadableChannel >pair is slightly more tedious to use. But in order to make the task >easier, you may if you want use the following convenience class, >provided in the sis-storage module: > >https://svn.apache.org/repos/asf/sis/trunk/storage/sis-storage/src/main/ja >va/org/apache/sis/internal/storage/ChannelDataInput.java > >Basically, you can give the ByteBuffer / ReadableChannel pair to the >constructor and either: > > * Invoke ensureBufferContains(int) for specifying how many bytes you > are going to need for the next sequence of ByteBuffer.get*() >operations; > * or use the convenience read*() methods when the number of bytes > needed is not known in advance. > > > Regards, > > Martin >
