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-289) Enable code sharing between Derby client and engine
Date Tue, 15 Nov 2005 23:19:50 GMT
Hi, Dan, thanks much for your review.  My responses below...

Daniel John Debrunner wrote:
> David W. Van Couvering wrote:
> 
> 
>>Hi, Dan, I get it now.  Yes, I think this is feasible, and I'll work on
>>that.  The network client code could do something like
>>
>>throw ExceptionUtil.newSQLException(SQLState.ERRCODE, arg1, arg2);
>>
>>Meanwhile, your inspection of and comments on the rest of the patch
>>would be much appreciated!
> 
> 
> I got confused due to the naming of the classes
> org.apache.derby.common.CommonFeatures and CommonInfo. Given the concept
> of shared code and the use of 'common' of the top-level package and
> source directory, I thought they were generic classes. But it turns out
> they make up a specific instances of a shared component. If you like a
> common shared component, or common common component. :-)
> 
> For instance, I started reading the code as if only one shared component
> could exist. Maybe I'm still confused, because CommonInfo is a specific
> instance of SharedComponentInfo but it uses a constant from
> CommonFeatures to implement its getMaxFeatureId(). The comments in
> CommonFeatures implies it holds the constants for multiple shared
> components, so why is MAX_FEATURE defined there, shouldn't it be
> specific to a shared component? And what is the benefit of having the
> constants in a single interface, why not have the feature constants in
> the shared component class, e.g. CommonInfo in this case? [Writing this
> paragraph I also was confused, it seemed like CommonFeatures would have
> code and CommonInfo would have the constants].
> 
> Looking to the future on this issue, would you expect the code for, say,
> a DRDA common component to be in the package org.apache.derby.common, or
> something like org.apache.derby.common.drda? The latter seems better,
> which would mean the common shared component code would logically move
> to org.apache.derby.common.common.
> 
> Maybe it would be better to stick to the single terminology of shared
> for the general concept, leading to java/shared/org/apache/derby/shared,
> and use common for the specific shared component that is common (to?).

Yes, I agree that this is confusing, and I am happy to change the 
top-level package to shared, so we would have

org.apache.derby.shared
   SharedComponentInfo
org.apache.derby.shared.common
   CommonInfo extends SharedComponentInfo

[and in the future...]
org.apache.derby.shared.drda
   DrdaInfo extends SharedComponentInfo

> The concept of max feature id must remain an implementation detail, not
> used by consumers of a shared component, as it won't work in the future
> if features are deprecated. The non-public aspect of getMaxFeatureId()
> in SharedComponentInfo seems to be the intent, but then to match that,
> the MAX_FEATURE constant needs to be not public.

OK

> 
> MessageUtil.EN is incorrectly named, and is not required, why not use
> java.util.Locale.US?
> 

I was using the naming convention used in the engine code, see 
MessageService.  I think it makes sense to use java.util.Locale.US, as 
long as you don't care that this is a departure from the old naming 
convention.

> For the MessageUtil classes, are all the public methods intended to be
> part of the public api, including methods like getCompleteMessage,
> formatMessage, composeDefaultMessage?
>

I'll take a closer look at these and see which ones make sense to be 
public.

> package.html and the comments in the classes are not in sync:
> 
> package.html: - Every time you add a
> new feature that is not forward-compatible, you need to add a new feature id
> 
> CommonFeatures: Every time a new feature is added that a user of a
> shared component depends upon, a feature id should be added here.


OK, thanks, will fix.

> 
> 
> Dan.
> 
>

David

> 
> 
> 
> 
> 

Mime
View raw message