db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel John Debrunner <...@debrunners.com>
Subject Re: Patch again again for DERBY-167.
Date Wed, 18 May 2005 13:22:56 GMT
TomohitoNakayama wrote:

> Hello.
> 
> 1st:
> 
>> The Derby "dblook" utility will have to be modified to account for the
>> "BY DEFAULT" keyword--right now, it generates all autoincrement
>> columns using the "ALWAYS" keyword.  See
>> org/apache/derby/impl/tools/dblook/DB_Table.java, in the
>> "reinsateAutoIncrement" method.  Luckily, this change should be fairly
>> easy given the changes in your patch.
> 
> 
> I see.
> I will modify dblook.
> Thank you for your notifying :D

The dblook changes could be a separate patch. Incremental development
is a good process, rather than requiring all the related changes to
be in one huge patch.


> 2nd:
> 
>> Second (minor):
>>
>> I noticed that in DefaultInfoImpl.java, the two new methods
>> (getDefinitionBitsValue and calcIsDefaultValueAutoinc) are declared as
>> static--is there a reason for that?  I made them non-static and
>> everything compiled, so I'm just wondering if this was intentional?
> 
> 
> Yes. That was intentional.
> 
> I prefer static method because it have narrower scope than instance method.

I agree, static methods are the correct type here.


I would encourage you (and anyone else) to keep any description (or
modified version of it) from the original patch e-mail in newer e-mails
with modified patches. This makes it easier for the committers to
understand and track patches. E.g. you last Derby-167 patch just had the
patch attached, you should keep any description from the first e-mail
with the patch. Another reason is that the description may have changed
due to comments etc.


In general the patch looks good, thanks for making changes for my
previous feedback.

Some minor comments on the patch:

In ResultSetNode and ColumnDefintionNode for this line you changed

if (defaultInfo != null && ! colDesc.isAutoincrement())

it would be good to add some descriptive comment. The issue here is that
the test is two negative conditions 'anded (&&)' together. Somewhat hard
for humans (well at least me) to understand quickly. A comment is also
useful for the original coder to help them ensure they have the correct
condition.

BITS_MASK_IS_DEFAULTVALUE_AUTOINC should be 'final' if it is intended to
be a constant.

In DefaultInfoImpl the calculating of definitionBits and
isDefaultValueAutoinc using those static methods seems too complex. If
there are ever a few more types to calculate we end up with a lot of
code to deal with it. Why not just have a single instance field
representing the type (matching your defintion bits), remove
isDefaultValueAutoinc field. Then the read/writeExternal methods just
write the type directly, and the isDefaultValueAutoinc method is
something like

+       public boolean isDefaultValueAutoinc(){
+               return (type & BITS_MASK_IS_DEFAULTVALUE_AUTOINC) != 0;
+       }


Dan.



Mime
View raw message