db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kristian Waagan (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-2346) Provide set methods for clob for embedded driver
Date Mon, 26 Mar 2007 13:37:34 GMT

    [ https://issues.apache.org/jira/browse/DERBY-2346?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12484126

Kristian Waagan commented on DERBY-2346:

Comments on 'derby-2346-only_for_review.diff':
  1) Wrong class name in the Apache header.
  2) Can ClobAsciiStream.writer be made final?
  3) Missing JavaDoc for exceptions thrown in EmbedClob-constructors
  4) In EmbedClob(DataValueDescrptor, EmbedConnection), should the SQLException be passed
to StandardException.newException to preserve the cause?
  5) In EmbedClob.length() there is a double try-catch with identical catch-blocks. I don't
understand why.
  6) EmbedClob.getSubString(long,int); another double try-catch as in e).
  7) Why is a new constructor added in UTF8Reader? (because of the streamSize?) Add JavaDoc?
  8) ClobUtf8Writer.flush() does not follow the contract described by the JavaDoc, where it
says it should throw an IOException if called after close. Even though flush() does nothing,
it would be good practice to throw it anyway.
  9) In ClobUtf8Writer.write(), should we preserve the underlying SQLException by calling
initCause or similar?
 10) Probably nothing severe, but maybe String.copyValueOf (static) should be used instead
of the String constructor?
 11) I'm having trouble understanding the arguments for LOBStreamControl.replaceBytes. Are
the longs positions for the LOBStreamControl, or for the passed in byte array? I would appreciate
some description of the arguments in the JavaDoc. If the methods is only supposed to replace
bytes, why do we need both a start and an end position?
 12) In replaceBytes, I can't see any consistency-checking for start-/endpos and incoming
buffer length. What am I missing?
 13) In replaceBytes, byte[] tmp is never referenced again. Can it be created directly in
the init-call?
 14) The two do-while(true) loop can both be rewritten to exit on a check in the while-clause.
The if (...) break can then be removed.
 15) Missing class JavaDoc in ClobStreamControl. Why is it needed? How does it differ from
 16) Is ClobStreamControl.conChild a final variable? 
 17) ClobStreamControl.getStreamPosition() needs more JavaDoc. I understand what it is supposed
to do, but the way it is implemented is hard to understand without comments.
 18) How does the code know it is hitting a character boundary (in UTF-8) when getInputStream(bytePos)
is called?
 19) Variable pos in ClobStreamControl.getStreamPosition() is never used.
 20) The if checking the in.skip-statements should be rewritten. In this case, as long as
skip does not return the same number of bytes skipped as requested, something is wrong.
 21) It must be clearly documented whether the various methods taking a position argument
expects this to be in number of bytes or number of characters.
 22) ClobStreamControl.insertString() seems to contain testing/debug code. Please revise it.
 23) General comment: Add more JavaDoc. Current status is there are lots of empty JavaDoc,
which is more frustrating than helpful. Also, all the empty return-tags will generate warnings.
 24) Is all this new/altered functionality covered by existing tests, or will there be new
ones written?

I plan to do another review of the synchronization policy later. If you can describe the intended
synchonrization policy used for LOBStreamControl and friends, it will be a lot easier to review
and verify.


> Provide set methods for clob for embedded driver
> ------------------------------------------------
>                 Key: DERBY-2346
>                 URL: https://issues.apache.org/jira/browse/DERBY-2346
>             Project: Derby
>          Issue Type: Sub-task
>          Components: JDBC
>    Affects Versions:
>            Reporter: Anurag Shekhar
>         Assigned To: Anurag Shekhar
>         Attachments: derby-2346-only_for_review.diff

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message