Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

RL2 CommandMap and EventCommandMap naming #77

Closed
darscan opened this Issue · 27 comments

7 participants

@darscan
Owner

I think it's a little annoying that most commonly used command map implementation is now called EventCommandMap. I can imagine users injecting ICommandMap and expecting to be able to use it.

CommandMap is actually just a base extension for other command maps (EventCommandMap, SignalCommandMap etc). So the current naming makes sense.

Still, for user friendliness, I think perhaps the base extension should be called CommandMapBase and the EventCommandMap should become CommandMap.

That way experienced RL1 users can continue to inject ICommandMap as per usual.

Thoughts?

@darscan darscan was assigned
@tschneidereit
@s9tpepper

I think the reasoning behind the name change makes sense.

I also like the descriptive nature of EventCommandMap, as it describes that it maps events to commands.

What if you kept with that same naming structure, you'd end up with:

SignalCommandMap
EventCommandMap
BaseCommandMap

It keeps the descriptive nature of EventCommandMap, but removes the ambiguous nature of plain CommandMap. And people looking for EventCommandMap will still find it, preserving familiarity w/ the framework for older users.

Either way its a good change, just a suggestion.

@JesterXL

Agree. For all the progress IDE's have made, FlashBuilder still isn't smart compared to IntelliJ about ordering most "probably" classes you meant to use. Additionally, for those who don't use IDE's with intelli-sense, and those like TextMate/Sublime etc. that try, the "don't make me think" matra works here; you see "Base" in the name, you know it's a base class.

@neilmanuell

just to throw a spanner, my immediate thought about CommandMapBase, is that it should be extended... Am I correct in saying all CommandMaps use it via a specific Trigger impl? in with case it could be a TriggerCommandMap or some variation?

+1 for the rest.

@darscan
Owner

Thanks all. @neilmanuell true indeed. It's not a base class, but rather a dependency of all command maps. Hmm..

@darscan
Owner

Ok, so regardless of the naming of the current CommandMap itself (which must change), how do people feel about the naming of the EventCommandMap?

Option A: CommandMap
Option B: EventCommandMap

Option A would be nice and familiar to Robotlegs users. Option B is more explicit.

@JesterXL

Option B: more explicit, allows others to use Signals, etc. in same project without name collisions.

@neilmanuell

Option B It IS rl2 after all, so I think some differences are expected. Explicitness helps indicate this. I opt for NOT having an ICommandMap: this will force new users to question and find the correct solution for their needs.

@neilmanuell

Btw. ICommandMap has always slightly thrown me. I often still type IEC... and wonder why I get nothing in my completion drop down, so dropping it (for me) will pose no problem.

@Stray
Collaborator

+1 for all the things you've all said. (Not that helpful, I know).

My heart says Option A, but my head says Option B.

I think this is a rare occasion in which all paths are equally valid. However, I do agree that the current 'CommandMap' is misnamed and needs to change. As Neil says, if there's nothing called ICommandMap / CommandMap then at least it's not possible to stumble blindly into trouble.

@darscan
Owner

Cool, so EventCommandMap it is. As for the naming of the core extension... I simply have no idea. CommandCenter?

@weltraumpirat

As a quick follow-up to the previous discussion (naming the base CommandMap) - is this going to be a valid map on its own, I mean: is it okay to use BaseCommandMap without extending it? If not, might I suggest to name it AbstractCommandMap instead of Base? I know ActionScript does not allow for "real" abstract classes, but by definition, an abstract class must not be instantiated, and can only be extended. And any method that must be implemented by concrete classes would simply be an empty stub that throws an error, so that it forces the developer to override it.

@darscan
Owner

The tricky thing here is that nothing extends it - extensions use or delegate to it - so it's not an abstract class.

It holds the Command-to-Trigger mappings... perhaps CommandTriggerMap?

@tschneidereit
@weltraumpirat

I think perhaps we should revisit the entire bundle of classes related to these mappings - when there's mappers, and mappings, and maps, and mapped, there's bound to be some confusion.

@tschneidereit
@neilmanuell

I like MappedTriggers as the class is the class that refs all Triggers that are Mapped. Also this is not an actor that will be used by application code, only by extensions. So the pool of confusion is smaller.

@neilmanuell

Actually is Trigger => Command implicit?

@weltraumpirat

Perhaps this goes too far, but taking a step back and looking at the entire command-mapping process, it seems to me like the name is not quite semantically correct: At the topmost level, what we do at present is to map events to commands - that is, we define state, but not action. What we are implicitly doing is, though, is telling an active mechanism to execute a command, i.e. to perform an action, when an event or signal (or whatever) is encountered.

In other words: We tell it to react to something. Therefore, shouldn't the mechanism more aptly be called IReactor? And wouldn't the API reflect its intent better like this:

reactor.reactTo( SomeEvent.SOME_TYPE, SomeEvent ).byExecuting( SomeCommand ).once();

We would then have an IReactor, implemented by EventReactor and SignalReactor ( or WhateverReactor ).

Moving on from there, I believe a Trigger, should rather be a Reaction, and the CommandTriggerMap would become the ReactionMap. I also think it would be wise to separate the IReactionMap interface (with methods map(), unmap(), getMapping()) from the IReactor interface ( with newly named methods addReaction(), removeReaction(), getReaction()), because we now clearly have two distinct mechanisms: Reacting to something, and mapping something.

What do you guys think? Is this completely out of the ball park?

@darscan
Owner

Very interesting indeed!

I think perhaps we should revisit the entire bundle of classes related to these mappings

I agree, many of them just emerged when writing the mapping DSL.

We tell it to react to something. Therefore, shouldn't the mechanism more aptly be called IReactor

I think there's something to this. However, surely almost everything in RL is a reactor of some sort then? The mediatorMap reacts to views landing on stage and creates mediators.

I also feel that EventCommandMap is a fairly comfortable name, and that people are accustomed to MediatorMap, CommandMap etc.

Perhaps a middle-ground worth exploring is to change the naming of the trigger stuff, but leave the *Maps. So, the EventCommandMap would register an EventReaction with the ReactionMap.

Anyway, I definitely think there's something here. I'm keen to collaborate if you feel like trying out some ideas.

@neilmanuell

surely this doesn't solve the problem, it justs changes the word Trigger to Reaction.

why not EventTrigger and TriggerMap.?

@weltraumpirat

@darscan Yes, a lot of things react to something in RL, but most of it is "under the hood", while in this case, the reaction is very prominent on the topmost level of abstraction. You could say it is the "motion" of the robot legs, as compared to the "parts".

Also, "mapping" views to mediators seems very natural, as the views are related to a physical location, the scene graph, and in this context, that metaphor just makes sense to me. The same is true for the injections, because they "map" dependencies to a location in the code, albeit on a different level of abstraction.

I did think about customs, too, and this is a really compelling argument - we will need to decide on whether 2.0 is enough of a jump to break them, and if the benefit of distinguishing the reaction mechanism from the other mappings outweigh the changes. I personally think that to continue using "map" in all of these different contexts is not the best possible idea.

I also considered IResponder, btw, but that term collides with the Flex interface, and that disqualifies it for my taste.

@neilmanuell The difference is only in the semantics: A Trigger is a mechanical part, while a Reaction is an activity. Also, a Reactor is clearly something very different than a Reaction and a ReactionMap; the names reflect how they are related. In comparison, EventCommandMap seems like a subclass of CommandMap, so this clearly needs to change for my taste.
EventTrigger sounds more like it triggers an event (as opposed to being triggered by an event), don't you think?

@neilmanuell

Agree that IResponder is out, but an EventResponse sounds good to me.
(I'm afraid Reaction is out, as it is the name of my first company and has certain memories associated with it.)

@weltraumpirat

Here's another thought: If we consider the top level class to be RL's CommandCenter, we could have an API like this:

commandCenter.execute( MyCommand ).on( MyEvent.EVENT, MyEvent ).once();
commandCenter.bypass( MyCommand ).on( MyEvent.EVENT, MyEvent );

The EventCommandTrigger would become an Execution, the CommandMap an ExecutionMap, and so forth.

@neilmanuell

I like Execution and ExecutionMap

@darscan
Owner

@weltraumpirat We initially had it that way around (command, trigger), but decided to stick to trigger -> reaction across the framework. For example: mediatorMap.map(Type).toMediator(Mediator)

I'm going to rename CommandMap to CommandCenter for now. We can come back to this after we've played with it for a bit.

@darscan
Owner

CommandMap renamed to CommandCenter: 8bbbf9e

@darscan darscan closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.