db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "TomohitoNakayama" <tomon...@basil.ocn.ne.jp>
Subject Re: Patch again again for DERBY-167.
Date Wed, 18 May 2005 14:36:26 GMT
Hello.

Daniel John Debrunner wrote:

>> 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.

Well ....
I just have started modification of dblook.
Then I will include it.
But if encountered some problem , I will postpone it for other task.


> 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.

Well ....
This condition is surely difficult.
Comment will be needed .
I will add.

Sometimes comment can be barrier for reading code.
Therefore, I hesitate to write comment.


> 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;
> +       }

I hesitated calculating  each time calling isDefaultValueAutoinc() method.
Well .... But its very small price.
Price of calculating each time will correspond to simple code.
I will modify it.


Best regards.


/*

         Tomohito Nakayama
         tomonaka@basil.ocn.ne.jp
         tomohito@rose.zero.ad.jp

         Naka
         http://www5.ocn.ne.jp/~tomohito/TopPage.html

*/
----- Original Message ----- 
From: "Daniel John Debrunner" <djd@debrunners.com>
To: "Derby Development" <derby-dev@db.apache.org>
Sent: Wednesday, May 18, 2005 10:22 PM
Subject: Re: Patch again again for DERBY-167.


> 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.
>
>
>
>
>
> -- 
> No virus found in this incoming message.
> Checked by AVG Anti-Virus.
> Version: 7.0.308 / Virus Database: 266.11.12 - Release Date: 2005/05/17
> 



-- 
No virus found in this outgoing message.
Checked by AVG Anti-Virus.
Version: 7.0.308 / Virus Database: 266.11.12 - Release Date: 2005/05/17


Mime
View raw message