commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benedikt Ritter (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CHAIN-98) Refactor command interface and base Implementations to enumeration to represent states
Date Wed, 03 Jul 2013 21:01:20 GMT

    [ https://issues.apache.org/jira/browse/CHAIN-98?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13699434#comment-13699434
] 

Benedikt Ritter commented on CHAIN-98:
--------------------------------------

Hi Jonas,

patch #2 has been applied, see [r1499551|http://svn.apache.org/r1499551]. Thanks for the great
work. This was surely not the easiest issue to start with :) I had to adjust a few things
a little bit:

 * added Apache License Header to the Processing enum
 * Removed some white spaces on empty lines
 * Changed CountCommand and ForwardCommand in chain-example2 and CatalogTest in chain-cookbook-examples.
Those were still using the boolean return value. You should always run mvn clean test, once
you are confident you have finished a patch (sometimes that reveals that you aren't ;)).

We still have work to do, so I'm not closing the issue. Here are the points that are still
missing/have to be adjusted:
 
 * The Filter interface's {{postProcess}} method still returns boolean. I think it would be
convenient if it would also return Processing
 * The semantics of {{o.a.c.chain.base.DispatchCommand.evaluateResult(Object obj)}} have changed.
The JavaDoc says that the implementation assumes that obj is of type Boolean. Before the patch
it simply casted to Boolean without type checking. So if for whatever reason some other type
would have been passed to that method a ClassCastException would have been thown. This seems
to be a reasonable protocol, given the documentation in the JavaDoc. Your patch changed this
and added a instanceof test. DispatchCommand now checks if it can cast and only then casts
to Processing. If obj is not of type Processing, continue is returned. I think if obj is not
of type Processing, something has gone wrong. So we should just let the ClassCastException
populate to the user. WDYT?
 * I think ChainBase shouldn't throw a ChainException if a Command returns null. IllegalStateException
fits better here, IMHO.

So if you got some spare time... :)
                
> Refactor command interface and base Implementations to enumeration to represent states
> --------------------------------------------------------------------------------------
>
>                 Key: CHAIN-98
>                 URL: https://issues.apache.org/jira/browse/CHAIN-98
>             Project: Commons Chain
>          Issue Type: Task
>    Affects Versions: 2.0
>            Reporter: Jonas Sprenger
>            Assignee: Benedikt Ritter
>             Fix For: 2.0
>
>         Attachments: chain-98-2.patch, chain-98.patch
>
>
> Base implementations should return the constants CONTINUE_PROCESSING or PROCESSING_COMPLETE
instead of returning true or false

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message