Tomohito Nakayama (JIRA) wrote:
I re-upload DERBY-326_2.patch.
I created patch *after* applying patch created executing "svn diff --diff-cmd diff
-x -uw".
Thank you very much for re-posting the patch without the whitespace
changes. It was much easier to read and understand this way. A few
comments below:
1) It was hard for me to understand the purpose of the class
ReEncodedInputStream.java simply from reading the code. I
think it would be good for you to add some comments describing
the purpose of this class and its expected uses.
2) It seems like it might be possible to improve these lines in
ReEncodedInputStream.java:
if(reader == null){
throw new NullPointerException();
}
if(enc == null){
throw new NullPointerException();
}
I think it would be better to pass a string to the exception
constructor, indicating which variable was null, as in:
throw new NullPointerException(
"null 'reader' passed to ReEncodedInputStream");
That way, if there are no line numbers available in the stack
trace, we'll still be able to tell the two exceptions apart.
3) In EXTDTAInputStream.getEXTDTAStream(), I did not understand
the purpose of the "is" InputStream variable. It seems to be
initialized to null, then checked in the finally{} block, but
is otherwise unused. Am I missing something? Or can this
variable be removed?
4) In EXTDTAInputStream.openInputStreamLazily, I think it would be
better to change the line
throw new IllegalStateException();
to
throw new IllegalStateException(
"Either 'blob' or 'clob' must be non-null");
5) Also in EXTDTAInputStream.openInputStreamLazily, what is supposed
to happen in the empty catch block for UnsupportedEncodingException?
Do you really intend just to ignore this exception? If so, then
I think it would be good to add some comments explaining why it
is appropriate to catch and ignore this exception.
6) Are the comments above DDMWriter.writeScalarStream() still
accurate? It looks like you have addressed some of these issues,
but you didn't seem to change the comment. It would be good to
check the comment and see if it needs to be updated.
7) It seems like the writeScalarStream method and its related methods
in DDMWriter.java (prepScalarStream, flushScalarStreamSegment, etc.)
do not use the "ensureLength()" protocol which is common elsewhere
in DDMWriter.java, but instead they examine the buffer length by
checking bytes.length and then being careful not to overrun that
size. I'm not sure exactly why these methods are different from all
the other data-writing methods in this respect, but I think it would
be nice to add some comments to the code describing this behavior.
Overall, I thought your changes were very good, and I think it will
be a definite improvement to avoid having to precompute the large
object length when returning it to the client.
thanks,
bryan
