Re: [tiles2] ComponentDefinitions - exposed implementation detail?

["David H. DeWolf" <dd...@apache.org>]

By the way, Antonio, I've looked at multidimensions at a high level, and 
realize that you're providing a unique implementation of this interface. 
  I meant to mention that I believe that you could accomplish the same 
thing (if we were to perform the refactor suggested below), by providing 
a customized DefinitionsFactory (that probably extends the 
UrlDefinitions factory).



David H. DeWolf wrote:
> I'm wondering why the ComponentDefinitions interface has been exposed 
> outside of the DefinitionsFactory.  To me, this class seems like an 
> implementation detail of the factory itself, and it should not be exposed.
> 
> Currently the interface is only used in three points outside of the 
> DefinitionsFactory (besides in tests):
> 
> 1) TilesFilter - used for refreshing the component definitions
> 2) TilesUtilImpl - used to share the instance with the world
> 3) ComponentDefinition - used to resolve hierarchy
> 
> I don't think any of these require the component definitions.  I'd like 
> to discuss refactoring them in the following manner:
> 
> 1) Encapsulate the refresh logic in the DefinitionsFactory.  The filter 
> will change to:
> 
> if(factory.refreshRequired()) {
>     // replace refresh logic with a call
>     // to the factory, removing the reference
>     // to ComponentDefinitions
>     factory.refresh();
> }
> 
> 2) TilesUtilImpl only exposes the ComponentDefinitions in order to allow 
> the Filter (#1) to access them.  This reference can easily be removed.
> 
> 
> 3) Encapsulate the hierarchy resolution within the DefinitionsFactory, 
> allowing the resolution to occur during initialization.
> 
> I believe that this refactor will remove some complexity and the 
> cyclical dependency which has been created between the 
> DefinitionsFactory and it's runtime environment.  IMHO, the 
> DefinitionsFactory should only be concerned with creating 
> ComponentDefinition objects, it should not need to know anything about 
> it's outside environment (see the current call to the Application Scope 
> map in order to re-retrieve the ComponentDefinitions).
> 
> 
> 
> David
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org

Re: [tiles2] ComponentDefinitions - exposed implementation detail?

["David H. DeWolf" <dd...@apache.org>]

Antonio Petrelli wrote:
> . . .
> So you are suggesting to put almost all the ComponentDefinitionsImpl 
> code inside UrlDefinitionsFactory? It is fine with me, but I don't know 
> what does Greg think of it :-)
> . . .

No, not necessarily. For now I was just going to make it a private 
member and ensure that it's not exposed externally.  That definitely may 
well be the next step though, I just hadn't thought of it yet ;)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org

Re: [tiles2] ComponentDefinitions - exposed implementation detail?

[Antonio Petrelli <ap...@apache.org>]

David H. DeWolf ha scritto:
> David H. DeWolf wrote:
>> I'm wondering why the ComponentDefinitions interface has been exposed 
>> outside of the DefinitionsFactory.  To me, this class seems like an 
>> implementation detail of the factory itself, and it should not be 
>> exposed.

Notice that before I committed SB-28 DefinitionsFactory class did not 
have a "getDefinition" method at all. Before this change, only 
ComponentDefinitions could return a ComponentDefinition.
Anyway, I think I agree with you, ComponentDefinitions is a way to store 
definitions, and its implementation is tightly connected to the 
DefinitionsFactory implementation.
Probably before the change (SB-28 again) ComponentDefinitions had some 
sense to exist, but at this point I think it is only one more layer.
But before cutting off ComponentDefinitions I think we should wait Greg 
that can explain better the role of ComponentDefinitions, probably 
ComponentDefinitions is the last class where the Locale object is part 
of the main API, since it has been removed from DefinitionsFactory by me...

>> 3) ComponentDefinition - used to resolve hierarchy

In fact the methods that use ComponentDefinitions in ComponentDefinition 
are deprecated (and never used), I think they can be removed.

>> 1) Encapsulate the refresh logic in the DefinitionsFactory.  The 
>> filter will change to:
>>
>> if(factory.refreshRequired()) {
>>     // replace refresh logic with a call
>>     // to the factory, removing the reference
>>     // to ComponentDefinitions
>>     factory.refresh();
>> }

Yeah much better!

>> 2) TilesUtilImpl only exposes the ComponentDefinitions in order to 
>> allow the Filter (#1) to access them.  This reference can easily be 
>> removed.

It's almost the change that I did in SB-28, I agree completely.

>> 3) Encapsulate the hierarchy resolution within the 
>> DefinitionsFactory, allowing the resolution to occur during 
>> initialization.

This will happen only with the default Tiles configuration files. With 
locale-specific files it will happen when a certain Locale is being used.

>> I believe that this refactor will remove some complexity and the 
>> cyclical dependency which has been created between the 
>> DefinitionsFactory and it's runtime environment.  IMHO, the 
>> DefinitionsFactory should only be concerned with creating 
>> ComponentDefinition objects, it should not need to know anything 
>> about it's outside environment (see the current call to the 
>> Application Scope map in order to re-retrieve the ComponentDefinitions).

So you are suggesting to put almost all the ComponentDefinitionsImpl 
code inside UrlDefinitionsFactory? It is fine with me, but I don't know 
what does Greg think of it :-)

> By the way, Antonio, I've looked at multidimensions at a high level, 
> and realize that you're providing a unique implementation of this 
> interface.  I meant to mention that I believe that you could 
> accomplish the same thing (if we were to perform the refactor 
> suggested below), by providing a customized DefinitionsFactory (that 
> probably extends the UrlDefinitions factory).

Yes, extending one class is better that two :-)

Ciao
Antonio

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org