search

robotlegs / robotlegs-framework

An ActionScript 3 application framework for Flash and Flex
https://robotlegs.tenderapp.com/
MIT License
966 stars 261 forks source link

Thoughts on the architecture #114

Closed creynders closed 11 years ago

creynders commented 11 years ago

I've been working on 2 extensions, swapping between them whenever I got stuck, which happened a lot. Partly due to my vague understanding of how everything works, but in a few cases also because I think some things are missing in the architecture.

For instance, if you take a look at commands, there doesn't seem to be a consistent way how commands are executed. Every extension (eventCommandmap and messageCommandmap) is responsible for executing them themselves. There's a lot of overlap (verifying it's a command, handling guards, hooks, mapping and unmapping the command) I understand why though, since the eventCommandMap needs to do other "stuff" in between those phases compared to the messageCommandMap. However, I think it would be a MAJOR improvement if there was some kind of CommandExecutor which you could reuse when writing an extension and decorate/extend/hook into its execution sequence. Or for instance if the CommandCenterExtension would provide a centralized execute method just as the commandmap in RL1 did. It wouldn't even have to be exposed in the API, but it would be nice to have something like it.

Speaking of commands in general: is it necessary to enforce the implementation of an execute method? Might a command not just as well be a class that implements a PostConstruct tagged method? I'd drop the whole describeType(mapping.commandClass).factory.method.(@name == "execute").length() thing (it's pretty slow too) and make the calling of the execute method only happen if the class implements a ICommand interface. The whole seems more flexible that way.

Then, I really miss factories or some kind of way to define which classes are created by default. As I already commented in the commandCenter extension it seems like a waste that the only thing preventing the reuse of the CommandMapper class is the fact that it creates concrete CommandMapping instances. The same applies to the ScopedEventDispatcherExtension. It seems a pity that EventDispatcher instances are created by default w/o any option to define another implementation of IEventDispatcher. (Obviously it's pretty easy to copy/paste the code from ScopedEventDispatcherExtension and create your own custom extension, but it also violates DRY)

I have a feeling RL2 is way superior to RL1 when it comes to "normal" usage, but in my (granted limited) experience when creating extensions it seems you need to overcome a lot more hurdles than before.

darscan commented 11 years ago

As always, thanks for the feedback - it's great to be talking about this stuff. I'm a little pressed for time this week, but I'll think about your concerns and get back to you soon.

creynders commented 11 years ago

Thinking some more on the factory-bit. How about using Injector#instantiateUnmapped? Obviously it's a bit limited since it would only allow for using one concrete implementation of ICommandMapping, IEventDispatcher,... which could give problems when using two extensions that might be using conflicting mappings.

darscan commented 11 years ago

Halloo! Sorry it's taken so long. Even now I only have headspace to address a couple of your points.

Speaking of commands in general: is it necessary to enforce the implementation of an execute method?

I quite like the idea of dropping the verifyCommandClass check, and instead only calling execute if the constructed object has such a method. I tried it out on a branch and it feels nice. However...

The added flexibility comes at the cost of "developer feedback". The current check happens at mapping time, and the stack-trace will lead you straight to the "problem". If we duck type the execute method, a simple mistake might not get caught - not at mapping time, and not at execution time either. Not sure how I feel about this. Thoughts?

There's a lot of overlap (verifying it's a command, handling guards, hooks, mapping and unmapping the command)

I see what you're getting at, but I believe that code can get "too dry". Sure, there's overlap, but once you put in hooks for every conceivable hook-point I believe the whole thing would be quite a mess. Also, considering performance I doubt there'd be a decent way to offer the flexibility that different extensions might need whilst running at a decent tick.

in the commandCenter extension it seems like a waste that the only thing preventing the reuse of the CommandMapper class is the fact that it creates concrete CommandMapping instances.

This is tricky. Even if you could return your own CommandMapping implementation you'd have to have to upcast it at some point to get at the extra info. In that case I think it might be better to bypass the CommandCenter entirely and just do the mappings yourself. Maybe I just don't see the use-case however. What, specifically are you needing?

Then, I really miss factories or some kind of way to define which classes are created by default.

This relates to the previous point. I think part of this was a design decision.. With RL2 I tried to make it so that there's never a good reason to extend via inheritance. I don't mind a little code duplication - it gives things the freedom to evolve independently. But again, I'm not necessarily seeing things from your side. It would be good to get some examples of the pain points.

creynders commented 11 years ago

No probs! I understand you're swamped with work.

The added flexibility comes at the cost of "developer feedback". The current check happens at mapping time, and the stack-trace will lead you straight to the "problem".

True, but in this case I think the trade-off is warranted. Most people will be implementing the ICommand interface or inheriting from the abstract Command class anyway. Those who don't will do this for a reason, compatibility with a different framework for instance, but they'll be attentive about it anyway. The benefit is huge for developers like myself, IMO. I work on tons of small projects, medium-sized at the biggest. Many of these projects are application prototypes or one-offs where the "rules" are a bit more loose. In almost all of my projects I have between 10 to 30 service classes with as many commands doing just one thing: directly calling a method of the service with no parameters or simply passing on mapped instances. If the execute check is dropped I can PostConstruct-tag those methods and wire them to the commandmap directly.

eventCommandMap( DeepThoughtEvent.DEEP_THOUGHT_BUILD, AnnounceAnswerToLifeTheUniverseAndEverythingService );

It goes against best practices I know, and definitely shouldn't be advertised as such, but it eliminates tons of direct-method-call commands. Obviously I could let the service implement an execute method, but that forces them towards a Command implementation and I want to give them maximum reusability. And this is just a small example of course, I'm pretty sure there are other use cases where it's beneficial to not be forced to implement an execute method.

I see what you're getting at, but I believe that code can get "too dry". Sure, there's overlap, but once you put in hooks for every conceivable hook-point I believe the whole thing would be quite a mess.

Agreed, but not in this case. There will be a LOT of commandmaps. In the current scenario all of them will be implementing their own version of guard and hook handling. While in 99% of the cases the reason why a commandmap extension is created has nothing to do with HOW the commands are handled, but WHEN they're executed. Already there is a discrepancy between how the EventCommandMap maps and instantiates a command and how @Pixels4Nickels SignalCommandMap does. There's a number of subtle rules on how a list of commands should be executed: guard-hook-command-guard-hook-command order for instance, but also instantiating it unmapped and mapping it as a value. Also some simple optimisation tricks like checking the length of hooks before attempting to apply them are easily forgotten. Extension developers won't always be aware of these. Commandmap users however will come to expect uniformness in all commandmaps. If there's a single default way of how commands are handled this is guaranteed. Extension developers will be able to deviate, but they'll know WHY they're deviating (and will be able to notify the users of their extensions subsequently). It'll be a feature of the commandmap, not an unwanted side-effect. And obvious reason nr 1, what if a really nasty bug is found with how commands are handled currently and there are already several commandmaps that copied the original code?

I took the liberty of starting on a command executor, you can find it here: https://github.com/creynders/robotlegs-framework/tree/command_executor https://github.com/creynders/robotlegs-framework/tree/command_executor-v1.0

It provides 4 callback moments; beforeGuarding, beforeHooking, beforeExecuting and afterExecuting. Last one I'll probably change to whenExecuted. Anyway, in the same branch I also modified the EventCommandMap to use the CommandExecutor, it only needs 2 of the provided hooks. I haven't had time to do execution speed comparisons yet, but hope to do them later today. It's a WIP, the tests aren't complete yet and there's a few things I need to think further through. Let me know what you think.

I've run out of time, I'll address the other points later today or tomorrow.

creynders commented 11 years ago

After a very quick command execution speed test, I got following results:

//EventCommandExecutor -- original impl trace( 'test_command_execution_speed_test', took );//2012-02-26: 2912, 2854, 2759, 2702 //EventCommandExecutor -- using CommandExecutor trace( 'test_command_execution_speed_test', took );//2012-02-26: 3933, 2969, 2920, 2758

I think we can ignore the first run of the one using CommandExecutor, since it deviates severely from the other 3. As you can see the difference is very, very small. This was a test with 100.000 commands mapped to a single event.

I'll push the stress test I wrote later today.

creynders commented 11 years ago

Ok, on to the rest...

In that case I think it might be better to bypass the CommandCenter entirely and just do the mappings yourself. Maybe I just don't see the use-case however. What, specifically are you needing?

I wanted to give an example of a commandsequencer I'm thinking about, but I think I found a better example, I was thinking about a PriorityEventCommandMap with something like this:

pecm.map( WeekdayEvent.ITS_FINALLY_FRIDAY )
    .toCommand( GoClubbingCommand )
    .withPriority( HIGHEST )
    .withGuards( HappyAsOnXTC )
    .withHooks( CallTheMissus )

But in the current implementation I'd need to rewrite practically everything that already exists in the EventCommandMap/CommandCenter, simply because I can't tell the CommandMapper to use a different implementation of ICommandMappingConfig instead of CommandMapping. If it were possible all I'd have to do is:

interface IPriorityCommandConfig extends ICommandMappingConfig{
    function withPriority( priority : int ): ICommandMappingConfig
} 

//IPriorityEventCommandMap
public function map(type:String, eventClass:Class = null):IPriorityCommandConfig;

Then create a concrete implementation of both. And extend CommandMapping obviously. But here's where I get stuck, since I can't alter the types of any of the instances created in the entire DSL flow, I'd need to create my own CommandCenter and CommandMapper, which would be 99.5% the same. It's not about the amount of work. At all. It just seems... wrong to be copying several perfectly fine classes, just because they create instances of a concrete type.

//EDIT I've been thinking about this from different angles:

pecm.map( WeekdayEvent.ITS_FINALLY_FRIDAY )
    .withPriority( HIGHEST )
    .toCommand( GoClubbingCommand )
    .withGuards( HappyAsOnXTC )
    .withHooks( CallTheMissus )

or even

pecm.mapWithPriority( WeekdayEvent.ITS_FINALLY_FRIDAY, WeekdayEvent, HIGHEST )
    .toCommand( GoClubbingCommand )
    .withGuards( HappyAsOnXTC )
    .withHooks( CallTheMissus )

But as far as I can see (which maybe isn't farther than my nose) the problem remains.

darscan commented 11 years ago

Awesome! Thanks for doing all this exploration.

OK, I'm sold on removing the execute check.

The fluent stuff is tricky though. Even if you could return an implementation of your choosing the fluent interface will still narrow the chain down to ICommandMappingConfig once you call any of the ICommandMappingConfig chaining methods - which will hide away your new methods.

Your last example gets around this. In that case we could use a factory for creating the ICommandMappingConfig, but later on (when inspecting the mappings) you'd need to upcast ICommandMapping to get at the special values, and that makes me sad.

Maybe you need to try it out and see what you run into.

The CommandExecutor looks good. I'm not sold on the function reference thing though. I think this is a good place to use the template method pattern. We can provide a default implementation and people can extend it if they need to override any of the methods. Funny, it's the pattern from RL1 that I was trying to get away from, but here I really think it makes sense.

I'll play around with some of these ideas over the next little while, and I look forward to seeing what you come up with.

creynders commented 11 years ago

Awesome! Thanks for doing all this exploration.

With pleasure, glad to be able to give a little something back for all those times I've had my ass and sanity saved by RL.

the fluent interface will still narrow the chain down to ICommandMappingConfig once you call any of the ICommandMappingConfig chaining methods - which will hide away your new methods.

Ah, yes of course. And thanks for pointing this out! When I read this sentence a few hours ago, I suddenly realized what really has been bothering me all along. I was mistaken, it wasn't about the concrete instantiation of CommandMapper, but about this: The usage of interfaces in the building process of the mappings gives an illusion of polymorphism, while in reality due to it's chained nature you're always locked-down to a specific implementation.

So I thought this through and I think I came up with a solution: (disclaimer: I'm severely pushing the boundaries of my limited analytical and programming skills, sorry if I overlooked something really obvious)

I'll first show what I was thinking about and move on to benefits and downsides later on


//--( CORE )--//
ICommandMappingBuilder
    //marker interface

ICommandMapping
    //since we're mapping commands this is really the only one that makes sense to have
    function get commandClass():Class;

ICommandTrigger
    //no changes here
    function addMapping(mapping:ICommandMapping):void;
    function removeMapping(mapping:ICommandMapping):void;

ICommandCenter
    //abstract interface
    function map(trigger:ICommandTrigger):ICommandMappingBuilder;
    function unmap(trigger:ICommandTrigger):ICommandMappingBuilder;

IDefaultCommandMappingBuilder extends ICommandMappingBuilder
    function toCommand(commandClass:Class):IDefaultCommandMappingBuilder;
    function withGuards(... guards):IDefaultCommandMappingBuilder;
    function withHooks(... hooks):IDefaultCommandMappingBuilder;
    function once(value:Boolean = true):IDefaultCommandMappingBuilder;

IDefaultCommandMapping implements ICommandMapping
    function get guards():Array;
    function get hooks():Array;
    function get fireOnce():Boolean;

//--( EventCommandMap )--//

IEventCommandMap
    function map(type:String, eventClass:Class = null):EventCommandMappingBuilder;

EventCommandMappingBuilder implements IDefaultCommandMappingBuilder
    private var _defaultBuilder  : DefaultCommandMappingBuilder;

    //wrapper methods

//--( PriorityEventCommandMap )--//

IPriorityEventCommandMap
    function map(type:String, eventClass:Class = null):PriorityCommandMappingBuilder;

PriorityCommandMappingBuilder implements ICommandMappingBuilder
    private var _defaultBuilder  : DefaultCommandMappingBuilder;

    function withPriority( priority : int ) : PriorityCommandMappingBuilder

    //wrapper methods

//--( CommandSequencer )--//

ICommandSequencer
    function add( commandClass ) : CommandSequenceBuilder

CommandSequenceBuilder implements ICommandMappingBuilder
    private var _defaultBuilder : DefaultCommandMappingBuilder

    public function before( CommandClass ) : CommandSequenceBuilder

    //wrapper methods

If I'm not mistaken, this should solve all of the above mentioned problems. It allows for a fluent interface, yet also allows for full reuse of existing components. It has an added benefit IMO, you can do this:

eventCommandMap.map( KillerBunnyEvent.RAAAAGGH, KillerBunnyEvent )
    .withGuards( HolyHandGrenadeShouldBeInPocket )
    .withHooks( ReleasePinFromHolyHandGrenade )
    .toCommand( ThrowTheHolyHandGrenade )

Apart from allowing reuse, this is aloso better IMO, for several reasons: 1/ you can (if you wish to do so ) configure in the same order as actions will be executed: guards, hooks, commands which is definitely more legible, IMO 2/ it makes more sense syntactically, to be able to swap all configuration steps 3/ it shows the way for a uniform mapping configuration 4/ it allows for both composition and inheritance. I too want to avoid inheritance, but sometimes, sometimes it just makes sense.

Now, the downsides: 1/ Substantial change. It should be non-breaking to framework and extension users, but breaking for extensions devs. But. There's no official release yet, so if you see any sense in this, now is the time to do the change. 2/ it can leave a configuration in a dirty state:

eventCommandMap.map( KillerBunnyEvent.RAAAAGGH, KillerBunnyEvent )
    .withGuards( HolyHandGrenadeShouldBeInPocket )
    .withHooks( ReleasePinFromHolyHandGrenade );

This is solvable however and I have a few ideas on this, but I'll explain them later on. 3/ A lot of interfaces and classes. A LOT. And I'm not too keen on marker interfaces myself, but in this case it makes sense IMO. Obviously this would need some thinking through on how to organize the files, to make the whole readable. 4/ it doesn't conform to how the other maps do their configuration. But there's a solution for that...

What do you think?

The CommandExecutor looks good. I'm not sold on the function reference thing though. I think this is a good place to use the template method pattern.

Great, I'll start working on it straight away!

creynders commented 11 years ago

Scratch all of the above. I start digging and experimenting and I think - maybe, hopefully, who knows - I found a solution. Or probably I just overlooked something, once more. I must say I'm in awe at how well you can overlook the entire flow, I have to get down and dirty to see if something's possible myself.

Anyway. I shifted my goals to: change as little as possible, especially in the API. And try to keep it simple, yet allow for reuse of the commandcenter parts either through composition or inheritance.

https://github.com/creynders/robotlegs-framework/tree/rewrite_commandcenter

Main changes: The CommandMapper no longer caches the mappings, I moved this to the triggers. TBH it makes more sense IMO, since mappings are maintained in a single place then. For instance, if I'm not mistaken, in the original implementation the mapper holds on to all CommandMappings until unmap, while fireOnce mappings are removed from the trigger after a single execution, correct? Not a bug, but not completely clean either. Now that the mappings are only stored inside the triggers, this is no longer a problem. Also, the CommandMapper is shortlived now. For each mapping attempt a mapper is created and it isolates the full configuration of the Mapping. In other words, it implements ICommandMappingConfig as well now and to the ICommandMapping interface I added setter meths. This should not be a problem since it's an extension developer facing interface anyway. The main reason I did this, is because it allows reuse of the CommandMapping class w/o locking it to the ICommandMappingConfig interface. I only added one interface: ICommandMappingFactory. It's used to allow the CommandMapper to create a CommandMapping w/o knowing the concrete type.

Now, this is an extremely rough draft. The code is messy. I didn't bother taking care of the ICommandUnmapper implementations, I just wanted to check out whether I could come up with something. And there's a huge number of tests that fails, since I didn't bother with updating the tests more than just making sure everything compiles. I also didn't bother with updating the MessageCommandMap (other than to let it compile) since its future is unclear. I added a hollow implementation of a PriorityEventCommandMap, just to get a feel how to use the rewritten commandcenter. It only uses one upcast, since the PriorityEventCommandMapper uses the CommandMapper compositionally, it needs to cast the mapping stored in the CommandMapper instance to the more extended IPriorityEventCommandMapping interface. That's it. It doesn't bother me, although I'm no fan of upcasts either, but in this case it makes sense, since it's an upcast to another (more specialized) interface, not a concrete implementation.

I dunno, maybe I'm stumbling in the dark here and missed something vital. If so, sorry for harassing you with this, and I promise if I completely missed the mark here I won't bother you about this again.

darscan commented 11 years ago

Hey! Sounds like you're on a roll. I cloned the branch, but it's a little hard for me to get my head into it with so many failing tests. I don't mind the failing MessageCommandMap stuff, but it would be good if the tests could show that the ECM still functions correctly (with unmapping etc).

Also, there seem to be two ways of storing mappings: linked list and vector. I think we should drop one (probably the vector based one). And, it would be nice to reduce the number classes/interfaces if possible.

Don't worry at all about bothering me with stuff. I feel bad that I don't have enough time to properly review all your changes. Also, the framework is fairly big and complex now, and I'm the only core team member left.. It would be cool if you got on board and "took over" the command related stuff. I'm not necessarily the best person for the job, as my style has changed quite a bit and I don't really use commands much any more. Not sure if you'd be keen to take it over, but maybe give it some thought?

creynders commented 11 years ago

I don't mind the failing MessageCommandMap stuff, but it would be good if the tests could show that the ECM still functions correctly (with unmapping etc).

Understood, I'll make them pass and add a few more to test some potential problems that I think haven't been covered yet.

Also, there seem to be two ways of storing mappings: linked list and vector. I think we should drop one (probably the vector based one). And, it would be nice to reduce the number classes/interfaces if possible.

Yeps, the vector one is used by the message command map, since the collection needs to be cloned for execution. But obviously if the message command map goes, it's no longer needed. TBH, I'm still not sure what the MCM is used for...?

It would be cool if you got on board and "took over" the command related stuff.

I'd love to, and it would be an honour!

I'll start working on the unmapping and updating the tests ASAP.

darscan commented 11 years ago

I'm still not sure what the MCM is used for

Exactly, neither do I (and I wrote it, so..).

I'd love to

Excellent!

creynders commented 11 years ago

Hiya,

unmapping implemented and tests updated to make 'm pass. Speaking of which. I'm no testing meister and TBH I find mockolate mightily confusing, I somehow just don't seem to be able to wrap my head around it. Anyway, I think there's some testing overlap between classes. I'll need to look into all of 'm and decide whether tests need to be moved. Your testing approach is somewhat different from mine, I grew up with a every-dependency-MUST-be-mocked mantra, but I've seen that sometimes you use concrete implementations as mock objects. Is this dependent on the relationship between the class-under-test and the to-be-mocked dependency? For instance when it's a composed dependency.

I commented out the MCM tests, to remove the noise of the failing tests. So, the CommandCenter and the EventCommandMap are fully updated and functional. The PriorityEventCommandMap is still a hollow compositional implementation, and next thing on my agenda is rewriting it to use inheritance, again just to get a feel for it. As an exercise I'll be updating the SignalCommandMap to see how that goes.

One more thing to note, at the moment I'm assuming each commandmap creates and holds on to a separate instance of the command center. This is due to the fact that if the commandcenter is a singleton, then two different but alike commandmaps will overwrite each others triggers, for instance the ECM and the PECM. At the moment they're manually constructed by the command maps, but maybe the CommandCenter should be mapped as a class to ICommandCenter and let the commandmaps retrieve an instance using the injector? Or we keep it a singleton and I just look for a way to have each command map uniquely identify their triggers.

it would be nice to reduce the number classes/interfaces if possible.

Definitely. Especially the command center API is quite extensive. There's two things I can do.

1/ drop the ICommandExecutor interface. Normally the only classes that rely on the command executors are the trigger implementations. They create the executors, so they're aware of their concrete type anyway. I see no real reason to have a ICommandExecutor interface except to provide a sort of this-is-the-way API.

2/ merge the ICommandMappingCollection and ICommandMappingIterator interfaces. Conceptually it makes sense to have them separated, but practically I think all ICommandMappingCollection implementations will implement the iterator interface anyway.

But that's about it, I'm afraid, I don't quite see what else could be dropped or merged.

One more thing. I'm thinking of creating a cache of verified command classes, but obviously this is semi-dependent on whether or not command classes are required to have an execute method. What are your thoughts currently on the hooking conundrum?

creynders commented 11 years ago

And coming back to this:

It seems a pity that EventDispatcher instances are created by default w/o any option to define another implementation of IEventDispatcher.

I'll take a look on how this could be solved w/o necessarily using a factory. I'd like to rewrite/port stray's relaxed event maps and it would be a lot easier and cleaner to do this with a RelaxedEventDispatcher implementation that decorates the standard event dispatcher.

Stray commented 11 years ago

@creynders Just chipping in because I'm glad you're picking this up and most code that uses mockolate will probably be stuff I wrote originally.

It may be that so much has changed that this is not relevant anymore - apologies if this is noise.

I tend to develop using a combination of end-to-end tests (integration tests) and unit tests, so there will be some tests that are very much behaviour tests - often where the implementation has changed considerably at times during the development, and we've wanted to ensure that there aren't regressions.

In order to not tie the tests to the implementation I tried to go in at a high level with the CommandMap itself and so on.

Hence the tests are not mocking every dependency (particularly composed dependencies) because we're testing that the behaviour of the system rather than the unit is correct.

Also, in many cases we started with the RL1 tests to ensure we had met those before introducing extra behaviour. I tried to change these as little as possible - in particular there is a lot of duplicate code in these, but that was preferable to introducing errors in the tests by editing them - we would have needed tests for the tests :/

Generally I tried - at least I think I did - to separate behaviour tests from unit tests, at least for the mediatorMap stuff. But the structure of the whole system as well as parts changed several times and so it's probably a lot messier than it should be.

If you want any specifics on the mockolate stuff, ping me on twitter - it was good timing that I left the project when I did because I've been pretty much floored by illness continually since then (withdrawl from Robotlegs causes serious symptoms it seems! ;) ). I'm semi-working from my sofa at the moment, so if you need anything explained give me a shout.

A RelaxedEventDispatcher sounds like a really good idea to me!

creynders commented 11 years ago

Hey @Stray

Sorry to hear you've been struggling with your health, hopefully you get well soon! Thanks a bundle for the explanation, it's largely what I already suspected. I'm going to try and maintain the same approach though, for consistency, but also because I can definitely see the value in it.

And yes, mockolate ... sigh I'll definitely be bothering you with questions on how to get certain things done, thanks for helping out. Oh, and "messy" is definitely not the adjective to be used for the original tests. On the contrary, they're really structured and clear. It's just that with modifying substantial parts some responsibilities have shifted to different classes and I need to untangle what tests make sense to be moved, where the blanks are and which tests should remain as-is since, as your wrote, they rather test system behaviour than unit correctness.

darscan commented 11 years ago

Hi hi. Just a quickie because today has been insanely long.

@Stray Wow, it's really been a hectic battle! Just got your TH mail though, and it sounds like things have taken a turn for the better. Holding thumbs. Also, thanks for taking a look at this, all things considered.

@creynders I only started getting into Mockolate recently. It's pretty awesome once you get the gist of it.

This is due to the fact that if the commandcenter is a singleton, then two different but alike commandmaps will overwrite each others triggers, for instance the ECM and the PECM

As far as I know, without looking at any code, this is only a problem with your new (key-based) design. Previously the trigger instances themselves were the keys (from the perspective of the Command Center), so there couldn't be any conflicts.

Regarding options 1 & 2: I'd say both.

I'm thinking of creating a cache of verified command classes

If the command classes are checked at mapping-time, then there is no reason to worry about performance, so I'd avoid that.

Still, I don't mind the new optional execute() approach. It does suffer some timing issues, as you pointed out before, but the whole motivation was to allow it for "advanced" use, so I'm fine with that. We can just add a note to the docs: You don't have to implement an execute() method, but beware that [PostConstruct] tags are invoked out of the normal sequence. Or something like that.

Ok, I must away to bed.

creynders commented 11 years ago

Heips,

Re: the command center, I'm nearing completion I think... In a nutshell: I isolated the DSL implementation into a separate class, CommandMapperFacade. Not sure about the name, though. It just configures the ICommandMapping instance and wraps a generalized CommandMapper instance, which does the real heavy lifting (retrieving the ICommandMapping instance from the trigger, etc.)

The rest of the structure remains roughly the same. The most notable changes are: 1/ the mapping of the command center as a type, instead of singleton, since it registers triggers based on keys provided by the commands maps. 2/ The addition of the ICommandMappingFactory interface.

EventCommandMap provides a nice overview of how to use the refurbished command center. Obviously the registration of triggers has changed since the command center changed. Further more it defines and creates all concrete classes that are used by the generalized command center components: EventCommandTrigger, CommandMapperFacade (there was no need to create a specialized EventCommandMapperFacade, since it uses the DSL as defined in commandcenter.dsl) CommandMapping( idem ) but the big advantage is that it's possible to create a non-inheriting DSL syntax, yet still use the default command center, command mapper etc.

I updated tests, and started out on creating one for CommandMapperFacade, but got a bit stumped on how to test it, since it composes the CommandMapper instance. Need to think on that.

Ah yes, another small change: I added a executeCommand global function which checks the optional "execute" method and dropped the verifyCommandClass function.

Overall I think it's an improvement and makes the command center a lot more flexible. I experimented with super-DRY, but the amount of classes and interfaces got a bit overwhelming. I think the current implementation provides a good balance. There's one thing that's nagging me: the EventCommandMap itself is responsible for returning a NullCommandUnmapper when trying to unmap a not found trigger. It means that all command map developers are responsible for doing that check.

Take your time with taking a look, there's no rush.

P.S.: just realized the Facade should've been called Adapter obviously...

creynders commented 11 years ago

I updated the SignalCommandMap to get a feel how to use the command center when developing a real extension. It went really smooth and fast. Hope you don't mind @Pixels4Nickels ... https://github.com/creynders/robotlegs-extensions-SignalCommandMap/tree/rewrote_signalcommandmap

pixels4nickels commented 11 years ago

I don't mind changes! In fact, I like the approach. But I would like to continue to maintain the extension, unless it is going to be included in the core RL2 extensions from here on out. So if you would send me a pull request that would be great. I will merge things and modify the tests as needed (if needed).

I have been following and enjoying the dialogue between you and Shaun, and have not chimed in because I think the changes are good(and I have little to add to the conversation at this point). I also think it is great to have you as a core contributor to help get the command stuff ready for release!

On another note- Shaun, are you not using commands at all now in your own implementations, or just using them more sparingly? If so, what kind of pattern/approach are you using to fill this area/duty?

Rock on, guys! This stuff is really looking great.

Ken

On Mon, Mar 18, 2013 at 11:38 AM, creynders notifications@github.comwrote:

I updated the SignalCommandMap to get a feel how to use the command center when developing a real extension. It went really smooth and fast. Hope you don't mind @Pixels4Nickels https://github.com/Pixels4Nickels ...

https://github.com/creynders/robotlegs-extensions-SignalCommandMap/tree/rewrote_signalcommandmap

— Reply to this email directly or view it on GitHubhttps://github.com/robotlegs/robotlegs-framework/issues/114#issuecomment-15073147 .

creynders commented 11 years ago

Hey,

Yes, of course you can keep maintaining the extension, it never was my intention to lift it out of your hands! I didn't issue a pull request (yet) since my command center changes aren't approved/merged yet and it could be there still are (major) modifications to be made...

darscan commented 11 years ago

Hey chaps. I don't think we can include the SCM into the official distribution until Signals hits an official v1.0 release. So, I imagine that @pixels4nickels 's repo will be the "official" one until then.

@creynders I played around with your fork this w/e. Good stuff. (btw, I squashed all your commits into one before I started reviewing, just to make it clearer).

I hope you don't mind, but it inspired me to try my hand at a completely different approach. I wanted to see if it was possible to drastically reduce the number of classes, but more importantly, the number of concepts in the system. Also, I wanted to see what a concrete command map might look like if pretty much all the responsibilities shifted into the Command Center.

I have a design sketched out, which I'll implement tonight/tomorrow and push into a branch for you to review.

Maybe it will work, maybe not. Perhaps it will give you some ideas that you can incorporate, or vice versa.

Anyhoo, getting back to your fork: There are a couple of things I spotted.

One is the linked-list implementation - querying first() actually modifies state inside the list. Sometimes this is OK, but often it violates QCS (http://en.wikipedia.org/wiki/Command–query_separation ).

This could be fixed by introducing a rewind() method, but I still wonder why the CommandMappingList has state at all. I think we start to lose the performance benefits of a linked-list once too much abstraction is introduced. At some point it ends up being quicker just looping through an array.

The other thing relates to long-lived state in the Executor:

https://github.com/creynders/robotlegs-framework/blob/rewrite_commandcenter/src/robotlegs/bender/extensions/eventCommandMap/impl/EventCommandExecutor.as#L57

The issue here is that an instance variable (like _eventConstructor) can be overwritten mid-execution by another call to execute(). This hasn't manifested yet because unmapPayload() is called before each command is executed, but if any of those commands have a [PostContruct] that triggers the same event the state will get mangled. The reason this wasn't an issue in the old code before was that the whole loop ran inside the scope of the execute() method. Even if that method was re-entered mid-loop, it would not overwrite any state in that closure.

A solution might be to pass mapPayload() etc as function closures into the the executeCommands() method, thus "closing over" the scope:

public function execute(event:Event):void
{
  const eventConstructor:Class = event["constructor"] as Class;
  if (_eventClass && eventConstructor != _eventClass) { return; }
  executeCommands(_trigger.getMappings(),
    function():void {
      _injector.map(Event).toValue(event);
      if (eventConstructor != Event) {
        _injector.map(_eventClass || eventConstructor).toValue(_event);
      }
    },
    function():void {
      _injector.unmap(Event);
      if (eventConstructor != Event) {
        _injector.unmap(_eventClass || eventConstructor);
      }
    });
}

OK, that's it for now.

I hope it's cool that I'm exploring another approach.. it may not work out, or it may be really nice. Maybe our solutions will converge. Regardless, your work here has been invaluable. IF it's not used directly please don't be discouraged. I know that I can be difficult to work with on this particular project. It's because I know the cost of a line of code in a library, and it's huge. Every line is a commitment.. ok, I'm blabbering.. I should get back to work!

creynders commented 11 years ago

I hope you don't mind, but it inspired me to try my hand at a completely different approach. I wanted to see if it was possible to drastically reduce the number of classes, but more importantly, the number of concepts in the system.

No, of course not. I've been taking a look at the RL1 extensions and trying to imagine new ones to think through how they could be implemented with the current and new implementation of command center. Just to say, I don't consider my current implementation as "final" at all.

The issue here is that an instance variable (like _eventConstructor) can be overwritten mid-execution by another call to execute().

Ah yes, of course. If I remember correctly I started out with the rule of having a new command executor for each dispatch, but lost that out of sight.

IF it's not used directly please don't be discouraged. I know that I can be difficult to work with on this particular project.

No problem at all. I just love doing this stuff. And I fully understand - and agree - that you want this to be as lean and clean as possible. It's what motivated me to start bugging you about the command center in the first place :)

Looking forward to see what you come up with!

creynders commented 11 years ago

So, your last post got me thinking and there's another approach I'd like to try too. I tried to think about the command center from the view point of extension developers for e.g. RelaxedEventCommandMap, SignalCommandMap, PriorityEventCommandMap (this last one is nonsensical, but it keeps reminding me to allow for non-standard command mappings) Basically all of those need to be able to:

  1. map a command to "something" e.g. event, signal, whatever.
  2. map and unmap a payload before/after command execution
  3. trigger the execution of a list of commands
  4. extend the command mapping
  5. (be able to manipulate the list of commands)?

All other behaviour is common to all of them. So I'm going to try and go for a decorator/strategy approach, where the various command maps are decoratable and use a strategy. The command center goes back to it's role of mapper/map creating factory.

I'm curious what I'll end up with :)

creynders commented 11 years ago

Aaah, that moment when all the pieces of the puzzle seem to fall in place as by magic. Or maybe I'm just not seeing the huge gaping hole right at the center of the puzzle...

Did a full rewrite: https://github.com/creynders/robotlegs-framework/tree/commandcenter_strategy

I drastically, and I mean: DRASTICALLY, cut the number of concepts, classes and interfaces. However, w/o sacrificing any of the new versatility. I did not bother with the unit tests, but I did make sure the event commandmap integration test fully passes. There's a number of small issues I'm aware of and a number of things I need to check, especially when it comes to the commandmappings vector structure integrity.

I'm really bad at naming things, so there's definitely a number of methods that need to be renamed. And one concept obviously needs to be renamed: ICommandMapStrategy and EventCommandMapStrategy.

Hope I'm in the right direction and there are no glaring conceptual mistakes. I'll start working on the unit tests and further cleanup after the WE.

creynders commented 11 years ago

Just had a thorough look-see at the rewrite, though there's still a lot of work in dotting the i's, I'm really confident this is the neat solution to all of the above...

darscan commented 11 years ago

Hey @creynders. I spent much of the week figuring out how to TDD up my idea. I've pushed it into a feature branch here:

https://github.com/darscan/robotlegs-framework/tree/crey-commands-inspired

Before checking out the Command Centre itself, it's probably worth having a look at how the Event Command Map looks with this approach:

https://github.com/darscan/robotlegs-framework/tree/crey-commands-inspired/src/robotlegs/bender/extensions/eventCommandMap/impl

I didn't have time to properly get stuck into your branch I'm afraid. Also, I didn't document mine very well, and it's a little.. unusual I think. I have to crash now, but I'll try to pop back soon to explain what I was going for. Also, I'm looking forward to digging into your strategy soon.

creynders commented 11 years ago

Hey @darscan I'm really curious, I hope I have time to take a look today.

My approach has evolved somewhat during the WE, be sure to pull before digging. I too think it's best you look at the EventCommandMap first. Now I feel I have some more grasp on what I was thinking, I can give a short explanation:

I really like it.

In one class you see whatever the hell mappings are mapped to (e.g. eventType + eventClass) You see what happens when a trigger adds/removes a first mapping (e.g. add listeners to a dispatcher) And also you have a very clear overview of how that command map hooks into the command execution process (e.g. maps/unmaps the event to Event and/or ConcreteEvent)

I am really, really satisfied with how everything turned out, it's clean and versatile. But I still need to take a look at your code, and again, I don't care what implementation we'll be going for, I have no problem whatsoever to drop this version if another one is better/cleaner/faster/more legible/whatever.

creynders commented 11 years ago

Ok, I just had a serious look at your approach. And I really like it as well! There's a number of areas where we clearly went in the same direction. Pros:

Cons:

I updated my version by stealing some of your concepts and fixing a bug with app-wide injectors being used for mapping events before command execution. Funny thing is that there's no more command center... I fell for the CommandExecutor again, and suddenly the command center dissolved, which is definitely a con in my current version, I think.

creynders commented 11 years ago

@darscan ok, I updated the SCM both with your version and mine: https://github.com/creynders/robotlegs-extensions-SignalCommandMap/tree/scm_darscan https://github.com/creynders/robotlegs-extensions-SignalCommandMap/tree/scm_strategy

From the POV of an extension developer yours is without a doubt easier to use and produces cleaner extensions. Yours got my vote!

darscan commented 11 years ago

Wow, awesome, thanks for doing that! Glad you like the approach. I'm still concerned about a couple of things:

ICommandClassMapper. The smallest requirement to be a mapper is the ICommandMapper interface, but in order to allow for different mapping APIs it can't have toCommand() or anything else in it. Hence ICommandClassMapper.. which is all pretty confusing I think. Maybe this isn't a problem. [note to self: investigate inner classes.. you'll remember]

Performance. function.apply() is slow, especially on methods. (In fact, it might be faster to supply a closure, and lose the parameter passing). However, I'm quite tempted to say: "if performance really is a concern you shouldn't be using a CommandMap.. just call a method! The CommandMap adds value - in terms of configuration, guarding, hooking etc - at the cost of performance. Don't push performance critical parts of your system through it". Of course, someone will still benchmark it against FrameworkX, but then they're completely missing the point, so who cares.

Sorting. There needs to be some way to prioritise the mappings.

If you think my approach is the way to go, then perhaps our next step is to build out a PrioritisedEventCommandMap. This would force us to sort out the sorting side, and reveal how many classes need to be added when creating one's own configurator. I'm not sure that we should ship such an extension, but it would be a good exercise. Maybe the ECM itself could become prioritised if it works out?

creynders commented 11 years ago

Hence ICommandClassMapper.. which is all pretty confusing I think. Maybe this isn't a problem.

No, you're right in that it is confusing. It would've taken me a lot of time updating the SCM if I'd not had the ECM as an example. That said, with the ECM as a guide, the update of SCM was really swift, even though some confusion existed, but I just coded by example. So, I don't think it's a problem.

(In fact, it might be faster to supply a closure, and lose the parameter passing).

And, in this case, it's more clear IMO.

However, I'm quite tempted to say: "if performance really is a concern you shouldn't be using a CommandMap.."

I agree. It's not what commands are for. They're the glue between the parts, nothing more.

Of course, someone will still benchmark it against FrameworkX, but then they're completely missing the point, so who cares.

Exactly.

Sorting. There needs to be some way to prioritise the mappings.

Yeps. Even though I like the linked list, it also keeps on bugging me - not implementation-wise - but because I don't "see" how to use it as an extension developer if I want to properly add sorting w/o potentially fucking up the command execution sequence at execution-time (e.g. un- and remapping an already executed command to a lesser priority will cause it to be executed again in the same sequence). If you start cloning a linked list to have an immutable list any of the benefits of using the linked list are severely outweighed by the cloning operation. BTW, about the immutable command sequence, IMO it would be better if you could map a command to the same event whose mappings are being executed, w/o it being executed immediately.

ecm.map('foo').toCommand(FooCommand);
dispatcher.dispatch( 'foo' );

//FooCommand
ecm.map('foo').toCommand(BarCommand);

In the current implementation BarCommand will be executed immediately, which I really don't like. I know it reflects how the std event dispatcher works, but nevertheless... (Note to self: write tests to lock it down as WAI-behaviour of the event maps)

EDIT: Hmm... there's actually quite some inconsistent and buggy behaviour hidden there. For instance:

eventCommandMap
    .map(CommandMappingCommand.EVENT_TYPE)
    .toCommand(CommandMappingCommand)
        .once();

//CommandMappingCommand
eventCommandMap.map(CommandMappingCommand. EVENT_TYPE )
    .toCommand( FooCommand );

will NOT execute FooCommand, since the CommandMappingCommand-mapping (hehe) is removed from the list, which makes its next return null. BUT

eventCommandMap
    .map(CommandMappingCommand.EVENT_TYPE)
    .toCommand(CommandMappingCommand);

//CommandMappingCommand
eventCommandMap.map( EVENT_TYPE )
    .toCommand( FooCommand );

In this case FooCommand IS executed.

Another reason to have an immutable command mappings list at execution-time, no?? To provide consisten behaviour.

That's why I wrote the last CC version with a vector instead of the linked list.

Perhaps our next step is to build out a PrioritisedEventCommandMap.

Excellent idea.

I'm not sure that we should ship such an extension

It's a can of worms we shouldn't open to others :)

creynders commented 11 years ago

So. I updated my version of the cc, by converging more towards your implementation. Main difference is I reintroduced the command center and took your idea of the factory meths for trigger and keys. Their implementations are in the strategy class.

I must confess, that at the moment I'm swaying towards "my" version (just referring to it as mine, so you know which branch I mean, obviously it's a complete merging of our both versions). It seems cleaner when it comes to the concepts, interfaces and classes, and if I'm not mistaken it should be just as easy to create a command map extension as with yours... I'll update my SCM implementation as well. Edit: yep, that went very smooth.

darscan commented 11 years ago

Ah yes, good point about the drawbacks of the linked-list. Vector it is then.

darscan commented 11 years ago

Had a very brief look through your branch. Will need more time to go through it properly. One thing jumped out:

https://github.com/creynders/robotlegs-framework/blob/commandcenter_strategy/src/robotlegs/bender/extensions/commandCenter/impl/CommandCenter.as#L58

It was very unclear to me what the callbacks were for, and that only one can be mapped per trigger etc. Wouldn't it be easier just to create them "inline".. just normal closures? But as I said, didn't have much time to dig in.. I'm probably missing something. Ah.. probably that first argument there (the trigger being passed through).

creynders commented 11 years ago

It was very unclear to me what the callbacks were for, and that only one can be mapped per trigger etc.

Yes. Finger. Sore spot. The reasoning is this: The strategy class's activate method is called when a trigger receives its first mapping to allow for listening to the event dispatcher for instance. However, the strategy class is a singleton and somehow it needs to maintain a mapping of each (event) handler to each trigger. At first I kept a map within the strategy class itself, but realized that all strategy classes would be copying this behaviour, so I moved it to commandcenter. I couldn't add the "real" handler as a listener, since that handler needs a reference to the trigger, that's why I came up with this pragmatic, not-so-elegant solution. Probably it's easier to understand if you compare it to: https://github.com/creynders/robotlegs-framework/blob/f0287ae79b9167053b842cc8045e836fec4fa16d/src/robotlegs/bender/extensions/eventCommandMap/impl/EventCommandMapStrategy.as

Anyway. I have a few options, I'll explore them today if I have the time. I got a feeling I'll be shoved towards dropping the strategy class altogether and moving closer and closer to what you have. Even though that seems to be the very obvious choice, I'm very hesitant. Again, due to extendability:

I've thought about 3 other extension concepts I think are really good to keep in mind, to ensure maximum reusability and flexibility.

  1. MacroCommand: at the moment I can imagine a trigger- and strategy-less MacroCommand which simply passes a vector of mappings to a command executor. Speaking of which, now I remember why I avoided passing the trigger directly to the executor, but a mapping list instead. On the other hand maybe the MacroCommand is a ICommandTrigger with a very bare bones strategy implementation. (I can imagine wanting to map a bunch of parameters as payload to all of the commands in the sequence)
  2. PayloadEvent: one of the things I went back-and-forth with in RL1 was switching between system-wide events and signals. I didn't like the huge number of signal singletons. Yet on the other hand I truly, truly hated that all my commands were tightly coupled to specific event classes. (At a certain moment I even created wrapper commands for all my "real" commands just to map the event payload separately. Craziness, pure and simple) So, for people like myself it would be good to have a PayloadEvent extension: it's a Signal, it's an Event... No, it's a PayloadEvent! Basically you declare the value object's classes in your PayloadEvent extending event class, and the PayloadEventCommandMap maps the event instance's value objects instead of the event itself. Now, what's interesting about this extension is that it's truly a decoration of the EventCommandMap. All it requires in se is to be able to override/decorate the payloadMapper and payloadUnmapper as declared by the ECM strategy. (Something which involves jumping through hoops at the moment, so I need to look into that)
  3. RelaxedEventCommandMap: this extension definitely needs to be made. Obviously there's a number of directions you could follow. Easiest would be to be able to decorate/extend the ECM again. This time either by overriding the activate and deactivate methods of the strategy and registering the listeners to a RelaxedEventDispatcher or by overriding the strategy's handleEvent method and caching the event to allow for relaxed retrieval within the activate method.

The last two extensions seriously suggest to me that it's a good idea to keep the strategy and the triggers separated and not move all the strategy's functionality to the triggers, since otherwise you'll be decorating each individual trigger instance. That doesn't sound as too much of a problem until you realize that if you combine those two extensions, both will be decorating each trigger instance. And who knows how many more extensions will take the same approach. The strategy allows me to configure command map behaviour once, instead of for every trigger. I think you know what I mean.

So, if you dig into my latest version some things may look as if they could be solved more directly, but I'm trying to keep all of the above in my mind.

creynders commented 11 years ago

So, I got rid of the strategy class. And, granted, the result is clean. Mr.- "No more sign of blown to pieces homies"-Wolf-clean. And all of the above... I realize I probably was aiming for super-DRY again.

Major achievements:

  1. No more weirdo callback creation shizzle
  2. No more trigger dependency in CommandExecutor
  3. A concept less
creynders commented 11 years ago

I created a proof-of-concept for the PriorityEventCommandMap: https://github.com/creynders/pecm

It literally took me 15 minutes. I'll take a look whether I can modify your version to use a Vector and use it to create a version of the PECM too.

creynders commented 11 years ago

Some thoughts:

creynders commented 11 years ago

Updated the PECM, SCM and the CC. Last 2 extensions have complete unit test coverage now, if I'm not mistaken.

Some updates to the CC ext:

I think people will be asking for both. If you think it's a bad idea, I'll drop it.

So, I think I'm nearing completion (hehe, seems like I've said that before) only thing missing is documentation, line-by-line inspection and git history cleanup (except if you'll be flattening it anyway, in which case I just leave it as-is). Oh, and your approval obviously :)

Maybe it's a junkyard, but to my eyes it looks real clean, robust and clear. But maybe I've been staring at it for too long.

Again, take your time. Let me know what you think.

Stray commented 11 years ago

Hey! Point me at the commits - I'd love to have a read, not from the point of approving/disapproving, but because I'm doing some thinking about diversity of approaches and stuff. What you've done sounds super interesting.

Personally I think release/detain wrapper is a good idea.

Stray

creynders commented 11 years ago

Hi @Stray fantastic, please do take a look! I'm standing between trees and suspect there's a forest here somewhere ... ;) The git histories are a bit of a mess, I'm afraid, but here are the repos:

  1. RL with CommandCenter + EventCommandMap
  2. SignalCommandMap
  3. PriorityEventsCommandMap Nonsensical, but really useful to make sure the DSL is decoupled from the concrete base classes.
  4. PayloadEvents extension Signal/Event hybrid

The last 3 extensions all are updated to use the experimental RL branch. Creating extensions is a breeze at the moment, however I'm banging my head against the same wall as in RL1: If I'd want an event command map that allows for relaxed payload events, with priorities, I'd need to create a separate RelaxedPayloadPriorityEventsCommandMap. I've already created a functional version of the CC that uses decoration instead of classical inheritance for the command trigger classes, but am stuck when thinking about how to stitch it altogether to allow:

  1. Having a separate PriorityEventsCommandMap and standard EventsCommandMap
  2. or, having a combined PriorityEventsCommandMap and standard EventsCommandMap

I would love it if developers were able to say I want the whole shebang combined into one single eventCommandMap or nicely keep everything separated. If you got any genius ideas, please do let me know!

creynders commented 11 years ago

Something like this would be wonderful, but I'm not even sure if it's überhaupt possible... I'm wrecking my brain. I'll try and go TDD on this one.

darscan commented 11 years ago

Sorry I've been so slack with feedback. I have been looking through your code, and will be setting some time aside tomorrow for a thorough review. I've also created yet another branch :) It's kind of a merge of my previous one and your latest:

https://github.com/darscan/robotlegs-framework/tree/crey-commands-trigger

in your version I saw you added a executeMethod to CommandMapping, is that something you want to pursue? Or were you just testing it out?

I'm pretty keen on keeping it.

OK, I have to run. Just thought I'd drop that link in the meantime.

creynders commented 11 years ago

@darscan after a side-by-side comparison these are my findings - all my opinions of course. I'm sorry if it seems negative towards your version, this is largely a matter of taste too I think.

I'm pretty keen on keeping it.

Ok. Why exactly? Is it to solve the problem of the post-construct methods running before the hooks? I do like it, I think!

What do you think of the idea of mix-and-matching the command maps? Is it worth checking out? I think it'll be pretty time-consuming that's why I ask up-front. Would be a shame to spend a lot of time on it, only to find out you hate the idea.

P.S.: I think you don't need the NullCommandUnmapper any more, right?

darscan commented 11 years ago

Hi! No problem, there are definitely a number of problems with my last attempt. BTW, I created another branch to test out event priority - just to see exactly how many classes/interfaces need to be created when one wants to change the fluent configuration API:

https://github.com/darscan/robotlegs-framework/tree/crey-commands-priority

(p.s. I didn't call it PriorityEventCommandMap.. I just modified the EventCommandMap itself)

I agree with many of your points, and I'll get back to you in a bit.

As for the executeMethod. This was something that I wanted ages ago and then forgot. It seems to me that this is exactly the kind of thing that "configurators" are for. No reason to force users to implement an execute method if we can let them choose which method to invoke.

darscan commented 11 years ago

P.S.: I think you don't need the NullCommandUnmapper any more, right?

Yes, but it reveals a bit of a "problem" (or nastiness) with our current approach. We don't need it anymore because you'll always get a mapper/unmapper back - even if one didn't previously exist. Also, the mappers/triggers are never removed - even when they're emptied. So, the very act of asking for an unmapper creates a new trigger.. which is not great. It doesn't really matter, as the trigger is only active when it has a mapping, but still.. a bit bleg.

darscan commented 11 years ago

In your version the command executor depends on the CommandMappingList, which in turn depends on a trigger.

Yeh, I don't like it either. At the same time I really want to keep the mapping list stuff in its own class. This could be fixed by using callbacks or events I suppose.

darscan commented 11 years ago

If I'm not mistaken, extensions that need to store any extra values for the mappings on top of the default ones will need to copy the command mapping class instead of extending it, since it's tied to the ICommandConfigurator class.

Yup, it forced me to copy quite a bit of DSL code when doing the Priority stuff. Agreed, not great.

darscan commented 11 years ago

All in all I think you're approach is good (also, I believe that our solutions are converging quite nicely).

Perhaps my main gripe with your branch is the fact that one needs to inherit from CommandTrigger. Inheritance creates a very awkward kind of contract, where the boundaries between responsibilities tends to get quite murky and hard to maintain over time. Another way of looking at it: the super class manages one set of responsibilities, while the child class manages another, but the interface between the two is not well defined (or defined at all). In these situations it's often worth pulling the super class out and giving it a clear role with a proper public API.

Looking at your abstract CommandTrigger now, it looks look almost everything it does relates to managing the list of mappings. On the flip side, the only things that the concrete EventCommandTrigger needs from it is the activate and deactivate hooks. (The PriorityEventCommandTrigger overrides map and unmap in order to sort the mapping list - but this could be done another way). So, if the super only really deals with the mapping list I'd prefer that that was extracted into a class with a well defined role (something like my CommandMappingList). Ultimately I think we can refactor that relationship and end up with your solution working exactly as it does now. Because that's not really where the main difference is..

Now that I look at it, I believe that the main difference is actually ICommandMapping. Your version makes it writeable, and that allows for composition that DRYs up the DSL stuff for specialised command maps.

I think if we kept that approach, employed the CommandMappingList and dropped the inheritance requirement, we'd end up with something pretty cool.