db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Van Couvering <David.Vancouver...@Sun.COM>
Subject Re: [jira] Updated: (DERBY-412) Connection toString should show type information and the meaning of the identifier that it prints
Date Mon, 01 Aug 2005 17:03:00 GMT
Hi, Tomohito, my responses inline

TomohitoNakayama wrote:

> Hello.
>
>>> point-1:
>>>    In the code of toString() in
>>> "java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java",
>>>    there exists four local variable which are declared and not used.
>>>    They are dbname, sessid, drdaid, txid .
>>>
>>>    I think they should be used or does not exist.
>>
>>
>> The local variables are used as part of the assignment statment for
>> connString.  I do this for readability, otherwise the assignment of
>> connString becomes fairly long and involved.  I suppose not using local
>> variables would save /some/ space, but I think it's important for
>> readability.  And note they are only created one time, the first time
>> toString() is called.  After that the entire string is cached in a
>> single variable and there is no cost in terms of time or space.
>
>
> Assighment statement for connString was not using local variables...
>
> +            connString =
> +              this.getClass().getName() + "@" + this.hashCode() + " " +
> +                lcc.xidStr +
> +                    
> lcc.getTransactionExecute().getTransactionIdString() +
> +                    "), " +
> +                lcc.lccStr +
> +                    Integer.toString(lcc.getInstanceNumber()) + "), " +
> +                lcc.dbnameStr + lcc.getDbname() + "), " +
> +                lcc.drdaStr + lcc.getDrdaID() + ") ";
>
> I think you should use local variable in this assginment statement.
> (or declaring new method for this assignment statement and save local 
> variable in toString() method)


Yep, you're right, I agree.  Thanks.

>
>
>>> point-2:
>>>    In the comment of toString() in
>>> "java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java",
>>>    description of unique id and lifetime of the connection and such
>>> was deleted.
>>>
>>>    I think they should be described in somewhere else and remained in
>>> the source codes.
>>
>>
>>
>> BrokeredConnection no longer has a unique id.  In the new version of
>> BrokeredConnection.toString(), all that's done is to print out the
>> classname and hashcode and the string representation of the underlying
>> connection.  I didn't feel this needed much of a comment.
>
>
> I found next mail.
> http://article.gmane.org/gmane.comp.apache.db.derby.devel/6720
> I understand BrokeredConnection no longer has a unique id.
>
> Then this point was solved.
> I think this part have no problem too.
>
OK

>
>>> point-3:
>>>    In
>>> "java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/checkDataSource.java",

>>>
>>>
>>>    there exists newly added instance method such as checkStringFormat,
>>> checkStringFormat, checkStringFormat, checkStringPrefix.
>>>
>>>    I think static method is prefered because it does not have scope to
>>> instance variables , if not much difficult.
>>
>>
>> I can do this, but can you explain further why static is preferred over
>> non-static? Multiple instances of this class aren't being created; is
>> there another reason why static is better?
>
>
> Instance method , which have scope to instance variable, would change 
> state of instance.
> Then it is more toroublesome than class method (static method) .
>
> Futhermore, if here comes needs to use these method in another classes,
> it is more easy for us to move class method to another class than to 
> move instance method.
>
> (And constructer of this class was declared as public.
> Then I think it is not guranteed that multiple instance of this class 
> is created.
> It is out of imagine multiple instance of this class was created in 
> other part,
> but trouble often happens in the situation that is out of imagine .....)

OK, I can change this

>
>
> Best regards.
>
>
> /*
>
>         Tomohito Nakayama
>         tomonaka@basil.ocn.ne.jp
>         tomohito@rose.zero.ad.jp
>         tmnk@apache.org
>
>         Naka
>         http://www5.ocn.ne.jp/~tomohito/TopPage.html
>
> */
> ----- Original Message ----- From: "David Van Couvering" 
> <David.Vancouvering@Sun.COM>
> To: "Derby Development" <derby-dev@db.apache.org>
> Sent: Saturday, July 30, 2005 3:53 AM
> Subject: Re: [jira] Updated: (DERBY-412) Connection toString should 
> show type information and the meaning of the identifier that it prints
>
>
>> Hi, Tomohito, thanks for your quick review of this patch.
>>
>> TomohitoNakayama wrote:
>>
>>> Hello.
>>>
>>> I have reviewed the patch.
>>>
>>> point-1:
>>>    In the code of toString() in
>>> "java/engine/org/apache/derby/impl/jdbc/EmbedConnection.java",
>>>    there exists four local variable which are declared and not used.
>>>    They are dbname, sessid, drdaid, txid .
>>>
>>>    I think they should be used or does not exist.
>>
>>
>> The local variables are used as part of the assignment statment for
>> connString.  I do this for readability, otherwise the assignment of
>> connString becomes fairly long and involved.  I suppose not using local
>> variables would save /some/ space, but I think it's important for
>> readability.  And note they are only created one time, the first time
>> toString() is called.  After that the entire string is cached in a
>> single variable and there is no cost in terms of time or space.
>>
>>>
>>>
>>> point-2:
>>>    In the comment of toString() in
>>> "java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java",
>>>    description of unique id and lifetime of the connection and such
>>> was deleted.
>>>
>>>    I think they should be described in somewhere else and remained in
>>> the source codes.
>>
>>
>>
>> BrokeredConnection no longer has a unique id.  In the new version of
>> BrokeredConnection.toString(), all that's done is to print out the
>> classname and hashcode and the string representation of the underlying
>> connection.  I didn't feel this needed much of a comment.
>>
>>>
>>>
>>> point-3:
>>>    In
>>> "java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/checkDataSource.java",

>>>
>>>
>>>    there exists newly added instance method such as checkStringFormat,
>>> checkStringFormat, checkStringFormat, checkStringPrefix.
>>>
>>>    I think static method is prefered because it does not have scope to
>>> instance variables , if not much difficult.
>>
>>
>> I can do this, but can you explain further why static is preferred over
>> non-static? Multiple instances of this class aren't being created; is
>> there another reason why static is better?
>>
>> Thanks,
>>
>> David
>>
>>>
>>>
>>> Best regards.
>>>
>>>
>>> /*
>>>
>>>         Tomohito Nakayama
>>>         tomonaka@basil.ocn.ne.jp
>>>         tomohito@rose.zero.ad.jp
>>>         tmnk@apache.org
>>>
>>>         Naka
>>>         http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>
>>> */
>>> ----- Original Message ----- From: "David Van Couvering (JIRA)"
>>> <derby-dev@db.apache.org>
>>> To: <derby-dev@db.apache.org>
>>> Sent: Friday, July 29, 2005 3:02 AM
>>> Subject: [jira] Updated: (DERBY-412) Connection toString should show
>>> type information and the meaning of the identifier that it prints
>>>
>>>
>>>>     [ http://issues.apache.org/jira/browse/DERBY-412?page=all ]
>>>>
>>>> David Van Couvering updated DERBY-412:
>>>> --------------------------------------
>>>>
>>>>    Attachment: DERBY-412.diff
>>>>
>>>> This patch contains the changes based on Kathey's feedback.  In
>>>> particular, I fixed BrokeredConnection.  I also verify that the
>>>> format of the connection strings is correct in the test code.
>>>>
>>>> derbyall passes all tests
>>>>
>>>> svn status output:
>>>>
>>>> M      java\engine\org\apache\derby\impl\jdbc\EmbedConnection.java
>>>> M      java\engine\org\apache\derby\iapi\jdbc\BrokeredConnection.java
>>>> M      java\engine\org\apache\derby\jdbc\EmbedPooledConnection.java
>>>> M
>>>> java\testing\org\apache\derbyTesting\functionTests\tests\jdbcapi\build.xml

>>>>
>>>>
>>>> M
>>>> java\testing\org\apache\derbyTesting\functionTests\tests\jdbcapi\checkDataSource.java

>>>>
>>>>
>>>>
>>>>
>>>>> Connection toString should show type information and  the meaning of
>>>>> the identifier that it prints
>>>>> --------------------------------------------------------------------------------------------------

>>>>>
>>>>>
>>>>>          Key: DERBY-412
>>>>>          URL: http://issues.apache.org/jira/browse/DERBY-412
>>>>>      Project: Derby
>>>>>         Type: Bug
>>>>>     Versions: 10.1.1.0, 10.2.0.0
>>>>>     Reporter: Kathey Marsden
>>>>>     Assignee: David Van Couvering
>>>>>      Fix For: 10.2.0.0
>>>>>  Attachments: DERBY-412.diff
>>>>>
>>>>> After the change for DERBY-243 the  connection toString() output is
>>>>> an integer which correspond to SESSIONID.  The output should
>>>>> identify the type and also the meaning of the identifier that it
>>>>> prints.  Perhaps a format that appends the default toString output
>>>>> with the sessionid information as it prints in the derby.log would
>>>>> be more informative.
>>>>> org.apache.derby.impl.jdbc.EmbedConnection@efd552 (SESSONID = 2)
>>>>> Ultimately this could be expanded to included other diagnostic
>>>>> information e.g
>>>>> org.apache.derby.impl.jdbc.EmbedConnection@efd552 (XID = 132),
>>>>> (SESSIONID = 5), (DATABASE = wombat), (DRDAID =
>>>>> NF000001.H324-940125304405039114{7})
>>>>
>>>>
>>>>
>>>> -- 
>>>> This message is automatically generated by JIRA.
>>>> -
>>>> If you think it was sent incorrectly contact one of the 
>>>> administrators:
>>>>   http://issues.apache.org/jira/secure/Administrators.jspa
>>>> -
>>>> For more information on JIRA, see:
>>>>   http://www.atlassian.com/software/jira
>>>>
>>>>
>>>>
>>>>
>>>> -- 
>>>> No virus found in this incoming message.
>>>> Checked by AVG Anti-Virus.
>>>> Version: 7.0.338 / Virus Database: 267.9.6/59 - Release Date: 
>>>> 2005/07/27
>>>>
>>>>
>>>
>>>
>>>
>>
>
>
> -------------------------------------------------------------------------------- 
>
>
>
> No virus found in this incoming message.
> Checked by AVG Anti-Virus.
> Version: 7.0.338 / Virus Database: 267.9.7/60 - Release Date: 2005/07/28
>
>
>

Mime
View raw message