Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Derby Development" <derby-dev@db.apache.org>
Received-SPF: pass (hermes.apache.org: local policy)
Message-ID: <004601c55bb6$fb52f2e0$2000a8c0@Arkat>
From: "TomohitoNakayama" <tomonaka@basil.ocn.ne.jp>
To: "Derby Development" <derby-dev@db.apache.org>
References: <000f01c55155$1d5bc270$2000a8c0@Arkat>
 <427A520C.3000509@Sourcery.Org> <001a01c5525b$2b1f72b0$2000a8c0@Arkat>
 <000c01c5548e$2fa726e0$2000a8c0@Arkat> <427FAFA7.6030705@Sourcery.Org>
 <427FC1BE.5090807@sbcglobal.net> <001101c55554$b4a28210$2000a8c0@Arkat>
 <098101c55566$99031b70$2000a8c0@Arkat> <098501c5556a$38d8bdf0$2000a8c0@Arkat>
 <c25576af050510100467e25527@mail.gmail.com>
 <000c01c5562b$bd08cc20$2000a8c0@Arkat> <4282A18B.9080304@Sourcery.Org>
 <001001c556f4$e3941970$2000a8c0@Arkat> <4285429C.2010006@sbcglobal.net>
 <003701c5588c$1fa267d0$2000a8c0@Arkat> <42893FE8.3010307@sbcglobal.net>
 <000d01c55b99$567c5580$2000a8c0@Arkat> <428B41B0.4090008@debrunners.com>
Subject: Re: Patch again again for DERBY-167.
Date: Wed, 18 May 2005 23:36:26 +0900
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; format=flowed; charset=ISO-8859-1;
 reply-type=original

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