I'm looking through the Configuration stuff in XWork, and have the following comments/questions:
* What does it mean for a package to be "abstract"?
* The "ConfigurationProviderFactory" is not a "factory", so it's a bad name. Also, what is it's purpose? It seems that the current code only allows for one ConfigurationProvider. Considering the need to build apps from subapps, which may use different config styles, that is a bad idea.
* This method in ConfigurationManager is wrong:
Interceptor getInterceptor(String clazz)
It assumes that there is only one instance of each interceptor class. This does not account for the case where one instance is used with many names (compare with servlets), and configured differently in each case. It has to change to:
Interceptor getInterceptor(String namespace, String name)
* the getAll* methods in the PackageContext's should not be there. The contexts may understand their own stuff, and that they happen to have a parent, but they should not be responsible for "putting it all together". That's the task of the ConfigurationManager IMHO. I'm not even sure a PC should have a direct link to it's parent. Maybe the name is enough, and the CM should make the connection.
* I can't remember if we resolved the issue of whether to use a strict parent/child relationship between PC's, or whether we should instead use a many2many "depends-on" relationship. I.e. with parent/child a package can only have one parent which it can use for accessing interceptors and interceptor packages, and for referring to other actions. With a "depends-on" relationship, where a package can depen on many other packages it is a much more open scenario, where a PC could (for example) depend on both the XWork "default" package, some company-specific "ourstuff" package, and then some "ourappdefault" package. This is useful for being able to reuse stuff to the maximum. Otherwise one would have to make sure that "ourappdefault" has "ourstuff" as parent which in turn has "default" as parent. And once you get to the point where you want to reuse two packages that you can't control you're in big doodoo. The classical multiple inheritance problem. Methinks the "depends-on" style is more useful, as outlined above. The rules for resolving names would still be deterministic though.
* Overall, I have trouble to (in JBoss-speak) "read the story". I can't see how things fit together, and how these classes are supposed to be used. There doesn't seem to be a coherent vision of what the goal of the configuration stuff is. I'm seeing lots of code, but no idea of why it's there. This is subjective, of course, but it's the feeling I get reading the code. I'm having a hard time relating it to any other configuration packages I've seen too, which will probably also make it harder for people (in general) to understand how it's supposed to work.
Some issues not related to configuration:
* It seems ActionInvocation's are called directly by code. A better way to do this is to create an ActionProxy which upon execute() creates an invocation and invoke it. This ensure that invocations are not reused. In general an "invocation" is not a good thing to deal with directly since it's conceptually a side-effect of something else. Hiding it behind an ActionProxy would fix this.
* The ActionInvocation seems to have a lot of stuff that is not really related to the invocation at all. The Action and result are invocation stuff, i.e. "local" (temporally) variables. The rest are more or less fixed and should go into the ActionProxy, and the invocation should then have a reference to the proxy. This removes a lot of code for copying parameters into the invocation as well (see the constructor). Similarly the code for init'ing the action should go into the ActionProxy. The invocation is just an invocation, nothing more.
That's what I could find so far. Any comments are appreciated, especially from Jason.
All in all, what I want to see is a refactoring that makes it easier to "read the story", and to easily see what the different players are and how the play together. Currently, as described, it is rather confusing.
regards, Rickard
ps. I really don't like the code style that is being used (and it's rather different from WW), but maybe that's just me.
------------------------------------------------------- This sf.net email is sponsored by:ThinkGeek Welcome to geek heaven. http://thinkgeek.com/sf _______________________________________________ Opensymphony-webwork mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/opensymphony-webwork
