You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by TomohitoNakayama <to...@basil.ocn.ne.jp> on 2005/05/19 14:35:44 UTC
I postpone modification to dblook.(Re: Patch again again for DERBY-167.)
Hello.
I decide to postpone modification of dblook.
Modification itself is not difficult.
However , it is needed to know all about SYSTEM table , to check
modification was correctly done.
It is a little hard for me to understand all about SYSTEM table right now.
//This may be suitable for my next task ...
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: "TomohitoNakayama" <to...@basil.ocn.ne.jp>
To: "Derby Development" <de...@db.apache.org>
Sent: Wednesday, May 18, 2005 11:36 PM
Subject: Re: Patch again again for DERBY-167.
> 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" <dj...@debrunners.com>
> To: "Derby Development" <de...@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
>
>
>
>
> --
> 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.322 / Virus Database: 266.11.12 - Release Date: 2005/05/17