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/java/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
