[
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 LOBStreamControl?
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.
Thanks,
> 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: 10.3.0.0
> 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.
