[chain] Provide a "dispatch" command

• Key: CHAIN-20
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Unassigned
• Reporter: Joe Germuska
• Affects Version/s: 1.0
• Fix Version/s: 1.1

Environment

Operating System: Mac OS X 10.0
Platform: Macintosh

Description

My team and I have developed a preference for the "DispatchAction" style in
Struts to cluster related behavior and decrease the number of classes. While
this is a matter of taste, it seems to be the taste of at least a few people in
the Struts community.

Is there any interest in providing an abstract base DispatchCommand which looks
up and invokes an internal method based on either a String property or the
String value of a context value? I started writing such a thing, and will
gladly finish it up, test it, and provide the code for more feedback if there's
interest.

If people think it's scandalous, we could probably just keep it for internal use...

Comments

[sean schofield]

Sounds interesting. Can you provide a few more details on how this would
work? Also, you mentioned it would be an "abstract base" command. I'm not
sure whether you meant that it would go in the "impl" package or not, but I'd
recommend it would go in the "generic" package instead.

[Joe Germuska]

(In reply to comment #1)
> Sounds interesting. Can you provide a few more details on how this would
> work?

you'd have a command something like this:

public class MyCommand extends DispatchCommand {

public boolean create(Context ctx) { // do some creating return false; }

public boolean update(Context ctx) { // do some updating return false; }

public boolean delete(Context ctx) { // do some deleting return false; }

}

You wouldn't implement "execute(Context)" because the base class has an
implementation of that method which does the lookup and invocation.

And then in your config, you could have
<catalog>
<command name="create" className="MyCommand" method="create" />
<command name="update" className="MyCommand" method="update" />
<chain name="delete">
<command className="MyCommand" method="delete" />
<command className="SomeOtherCommand" />
</chain>
</catalog>

> Also, you mentioned it would be an "abstract base" command. I'm not
> sure whether you meant that it would go in the "impl" package or not, but I'd
> recommend it would go in the "generic" package instead.

Good, because I put it in "generic" so far :^) "Abstract" just because it would
do nothing unless you extended it, although whether or not an "abstract"
modifier was actually applied to the class declaration isn't so important since
there wouldn't be any abstract methods.

[Joe Germuska]

Created an attachment (id=13888)
Incomplete draft of a Dispatch Action - totally untested

Just to have something to look at...

[Joe Germuska]

Created an attachment (id=13890)
Slightly revised, tested, and more documented DispatchAction

[Joe Germuska]

Created an attachment (id=13891)
test case for DispatchCommand

[sean schofield]

+1 for this idea (now that I understand it).

Basically the idea is to allow for the use of a single command with some
private methods that can be shared by your dispatch methods right? At least
that's how I use DispatchAction ...

You'll also need to modify the digester configuration stuff. Maybe you could
extend or modify ConfigRuleSet?

We're doing something like this in our app. We've got several commands that
can handle various field changes to the form. These commands have methods
that are specific for each of the fields that might change (and that they're
interested in.) The parent (abstract) class for these commands has an execute
method which calls a getRegisteredFields() method (implmenting by the
subclass). It then calls any appropriate methods using reflection.

Its not exactly the same as what you're doing I know, but I just wanted to put
that example out there.

[Joe Germuska]

(In reply to comment #6)
> You'll also need to modify the digester configuration stuff. Maybe you could
> extend or modify ConfigRuleSet?

I'm not sure what changes need to be made; this should work as any command.
I'll admit that my testing was just in JUnit and not in a running chain
environment using XML config, but I think there's nothing special that digester
needs to know about.

[sean schofield]

How else would the setMethod be called (ignoring the context case)? Btw, I'm
kind of opposed to the context version. I think its better to make the user
explicitly identify the methods upfront in the XML. I'm of the opinion that
less in the Context is better but thats JMO.

One other suggestion. Maybe change the method to setDispatchMethod or
setMethodName? The phrase "getMethod" has other connotations.

I will try and take a look at the configuration later tonight.

[Joe Germuska]

Because the digester rules for <command> include "SetProperties", any XML
attribute in the <command> element whose name is a bean-property-name of the
instantiated command will be used to set that property value with the
attribute's value. So:

<command name="foo" className="MyDispatchSubclass" method="doFoo" />
would execute MyDispatchSubclass.doFoo(...)

I don't feel strongly about the naming of the properties.

I included the methodKey version mostly to keep consistency with other patterns
I've seen in early Chain development. It seems similar to the Struts
DispatchAction which allows a request parameter value to be used as the
specification for dispatch. I don't use that myself, but it seemed like some
folks might want such. It would be possible to achieve the same but more
verbosely with a number of explicitly configured commands and a CopyCommand, so
I wouldn't argue too much over it.

[sean schofield]

OK sounds good. I wasn't aware that SetProperties was being used on the
command element (its hard for me to check source code at work.) You make a
good argument for the context thing (even though it sounds like you agree with
me that its not the best practice.)

[sean schofield]

A couple of additional (minor) comments for you. I'm not sure if the evaluate
method is really necessary as a protected method. Whatever dispatch method is
called should evaluate to true/false. I don't know that it makes sense to
allow subclasses to get too crazy with the return parameter on this.

I would also recommend making the method, methodKey and methods properties all
private (instead of protected.) The first two are just properties and already
have public getters and setters. It also seems risky to allow access to the
WeakHashMap to the subclasses.

[sean schofield]

Are we ready to commit this? I have some cool stuff in the works once we do.

[Joe Germuska]

I'll do it myself, as soon as I re-checkout the repository in read/write mode
and look harder at your last couple of suggestions...

[Joe Germuska]

With all due respect, Sean, I decided to leave things as they were. Having
"extractMethod" protected is not very useful if the methods cache isn't
available (well, it would be annoying to have to replicate it). And while the
evaluateResult(...) may be surplus, I don't see that it hurts anything.

[sean schofield]

No worries. I'm looking forward to playing with it.

[Niall Pemberton]

I agree with Sean's comment about making method, methodKey and methods properties all private (instead of protected).

There are two issues with it - firstly its exposing the Map implementation which from the FastHashMap experience in other components could be a PITA later and secondly it sets the visibility for all descendants which can't then be changed. Far better IMO to provide methods to access the cache rather than expose it this way.

I just posted about this on the dev list (chain 1.1 release?) since the checkstyle report highlighted it - should have looked here first

[Joe Germuska]

i'm fine with making all the actual properties protected. Are you going to do this? Do you want me to?

[Niall Pemberton]

assume you meant private - I can do this when I get back from holiday (after Monday) but feel free to jump in if you want.