Hallo Bene, > * JavaDoc of Command says that commands have to be designed in a > thread-safe manner, yet all the base implementations are not thread safe! I > have created CHAIN-96 [1] for this, because this definitely has to be fixed > before 2.0
+1 > * The same is true for o.a.c.chain2.Chain and its base implemenation. For > example there is a race condition in o.a.c.chain2.impl.ChainBase.add(CMD). > The method first checks the forzen flag and then acts... Another thread may > already have altered frozen. I have created CHAIN-97 [2] for this. ok, gut! > * The use of Context<K, V> is inconsistent throughout the API. Commands > only need a C extends Map<K, V> for execution but the RemoveCommand > implements Command<K, V, Context<K, V>>. Why do we need the Context > interface anyway? It doesn't add much, the only method is just a generic > wrapper around Map.get(Object). Context must have disappeared from methods signatures some time ago, it is maybe a refuse - but at that time we agreed on creating a commodity interface which contains the <T extends V> T retrieve(K key); method; > * Base implementations should return the constants CONTINUE_PROCESSING or > PROCESSING_COMPLETE instead of returning true or false I'd even switch to enumeration to represent states... > * It may make sense to introduce an enum instead of working with booleans > as return values for commands. hahah nice one, I wrote the sentence above without reading the last bullet, sounds we have an agreement here... :) Thanks & Best, -Simo http://people.apache.org/~simonetripodi/ http://twitter.com/simonetripodi On Thu, Jun 20, 2013 at 1:01 PM, Benedikt Ritter <brit...@apache.org> wrote: > Hi guys, > > I had a look at the command interface and it's base classes yesterday, and > I think there is some work to do :) Here are the points I found: > > * JavaDoc of Command says that commands have to be designed in a > thread-safe manner, yet all the base implementations are not thread safe! I > have created CHAIN-96 [1] for this, because this definitely has to be fixed > before 2.0 > * The same is true for o.a.c.chain2.Chain and its base implemenation. For > example there is a race condition in o.a.c.chain2.impl.ChainBase.add(CMD). > The method first checks the forzen flag and then acts... Another thread may > already have altered frozen. I have created CHAIN-97 [2] for this. > * The use of Context<K, V> is inconsistent throughout the API. Commands > only need a C extends Map<K, V> for execution but the RemoveCommand > implements Command<K, V, Context<K, V>>. Why do we need the Context > interface anyway? It doesn't add much, the only method is just a generic > wrapper around Map.get(Object). > * Base implementations should return the constants CONTINUE_PROCESSING or > PROCESSING_COMPLETE instead of returning true or false > * It may make sense to introduce an enum instead of working with booleans > as return values for commands. > > Thoughts? > > Benedikt > > [1] https://issues.apache.org/jira/browse/CHAIN-96 > [2] https://issues.apache.org/jira/browse/CHAIN-97 > > -- > http://people.apache.org/~britter/ > http://www.systemoutprintln.de/ > http://twitter.com/BenediktRitter > http://github.com/britter --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
