ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <cost...@covalent.net>
Subject Re: ProjectHelper comments
Date Mon, 11 Mar 2002 17:02:09 GMT
On Tue, 12 Mar 2002, Conor MacNeill wrote:

> I've done cleanup on the ProjectHelper changes and I have some 
> comments/questions. I'm not sure how much a pluggable ProjectHelper 
> implementation is required.
> 
> 1. I don't know why ProjectHelperImpl extends ProjectHelper. I had expected some 
> sort of separate abstract class or interface that the ProjectHelperImpl would 
> extend and which other ProjectHelperImpls would also extend. Right now 
> ProjectHelper is acting as both the factory and base class for ProjectHelpers. I 
> think it would be better to separate this out. Since the previuous 
> implementation of ProjectHelper kept most methods private this should not be 
> difficult.

We could have a ProjectHelperFactory, to create the ProjectHelper 
instance. But do we need it ? My feeling is the current ProjectHelper is 
simple and clear enough - it has few static methods ( for convenient 
access to various introspection features ), the (abstract) parse() method
that will be implemented in subclasses, and the code to find and load
the real implementation. 

Given that this abstraction will be used in few very specialized cases
( to completely replace the xml processor ), I don't think we need to 
make too much complexity out of it. 


> 2. Why has parse become public? There is no need for this to be public. In fact 
> the old ProjectHelper only exposed configureProject (beside the utility methods)

IMHO the 'right' way to use ProjectHelper is:
  ProjectHelper impl=ProjectHelper.getProjectHelper();
  // set/query impl options ( none for now )
  impl.parse();

Static methods are nice sometimes, but both mechanisms are valid and many 
people feel more comfortable using instance methods. My preference would 
be to deprecate configureProject ( i.e. recommend people to avoid it ), 
but I don't feel very strongly about it. 


> 3. Why is getProjectHelper public? There is no need for users of Projecthelper 
> to get the actual ProjectHelper instance being used. All they should be using is 
>   configureProject()

See previous. 

If needed, we might have to add other methods, like isSax2Processor() or 
isXmlBased().

Or createProject() - to allow the helper to use a specialized Project 
impl.
  
> 4. Why is ProjectHelper's constructor public?

I changed it to protected.
 
> 5. If the user has specifically specified a Projecthelper instance via a 
> property I don't think it should be ignored. The following should probably be fatal.
>          } catch (SecurityException e) {
>              // It's ok, we'll try next option
>              ;
>          }

Ok, that covered the case when ant is run in a sandbox and it doesn't have
permissions to read the system properties - probably doesn't make too much 
sense in the case of ant, but it was quite important for Jaxp ( from which
I cut the discovery code ). 

The idea was to fall back to the next discovery mechansim if the previous
fails - but in ant's case you are right, failing is better ( since the 
various implmentations of ProjectHelper are not equivalent ).

I fixed that, thanks.

Costin


--
To unsubscribe, e-mail:   <mailto:ant-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:ant-dev-help@jakarta.apache.org>


Mime
View raw message