Create Mediator immediately when pass instance of a view class

Cotton Hou's Avatar

Cotton Hou

04 May, 2011 04:41 AM

Let's say I want to map a view instance manually, because this view may not going to be added to display list ever for some reason, below is the step currently:

var view:View = new View();

mediatorMap.mapView(View, ViewMediator);
mediatorMap.createMediator(view);

Wouldn't it be more handy just by passing the instance of the view class and create those Mediator automatically?

mediatorMap.mapView(new View(), ViewMediator);

To do so, I've made a little change on the MediatorMap class, and come with test case associated, suggestion will be helpful and thanks for your time.

https://github.com/imcotton/robotlegs-framework/compare/da548c1...d...

  1. Support Staff 1 Posted by Stray on 04 May, 2011 08:08 AM

    Stray's Avatar

    Hi there,

    this is interesting - thanks for contributing. I hope I'm not going to come over as too negative.

    I know this change doesn't technically break semantic versioning because of that * typed first parameter in the function - but the purpose of that viewClassOrName: * was to allow you to make mappings for Classes that aren't compiled into the same swf as this code, and will only be loaded at runtime.

    If there was a more elegant way to define this in AS3 then we would, and that * would definitely just be "Class".

    My gut is that replacing 2 lines of code that express your intent clearly with one line of code that is less clear isn't a saving - 'less code' shouldn't be our primary focus. But it's good to bounce ideas around anyway - and certainly the joy of open source is that if this is how you want it to work you can work with your changed copy instead.

    Personally I wouldn't trade a single line for an extra (invisible) path in the code. I like it to be very obvious what my code is doing in each line.

    For example - in the CommandMap, it's possible to keep your Command alive just by using non-weak listeners and mapping some events... but it's much clearer to use the detain and release methods to show other developers what your intent was.

    My other worry is that normally a 'mapping' is a change to future behaviour of your code, and it persists. It usually has a symmetrical 'unmap' version that undoes what you've done.

    I think using mapView in this way breaks that pattern - and there's some advantage to being consistent.

    I also have some concerns that this will blow up (I don't think the test is robust enough, you've got an error waiting to happen there!) but I've commented on the commit directly about those.

    Sorry to sound negative - I'm very biased towards clear rather than short code :) I could be in the minority!

    What do others think?

    Stray

  2. 2 Posted by Cotton Hou on 04 May, 2011 01:36 PM

    Cotton Hou's Avatar

    Hi Stray,

    Thanks for reviewing, even the "negative" feedback is welcome as well :)

    I think the viewClassOrName:* seems not self-description enough after making this change, it could change to just value:* at the future for naming issue.

    The goal of passing this arguments is to get the Class reflection info in the end, which only come to a question of whether to make the function flexible enough, for instance we can still change to concrete type of Class and let all the reflection work done outside for type constraint purpose.

    Actually part reason of making this change is want to pay attention to the fourth parameter autoCreate:Boolean = true also, and by calling the method with letting default to true is pretty much obvious given out the permission.

    And for "unmap", at this point I seems doesn't need to do any change for, because the original behavior already done the job well, which get the Class name at the first step whether passing the Class type or instance of Class. So if you looking this way, this "mapView" call is got consistent with "unmap" finally...

    At the end, I've made several change after your comments on my commit, but the diff link still working in my first post above, please have a look, and always welcome to getting feedback and thanks for your time.

  3. Support Staff 3 Posted by Stray on 04 May, 2011 01:48 PM

    Stray's Avatar

    Thanks Cotton,

    what's interesting is that your fixes for the problems I found (the errors) start to cause a lot of extra nested logic.

    To me, this is usually a sign that you're trying to use a single method to do two things :)

    My bigger concern though is that 'map' and 'unmap' in the Robotlegs context are about creating and clearing rules. What you're now doing is using 'map' to mean something different - it's that inconsistency that really bothers me.

    Do you see the difference?

    Stray

  4. 4 Posted by Cotton Hou on 04 May, 2011 02:23 PM

    Cotton Hou's Avatar

    Yeah, the nested logic it seems bad taste at the moment, I think it could be clear by doing a code refactor, but before the main repository going to merge the change in, for avoid too many diff change goes around.

    And actually if you try to map the "contextView" of each Robotlegs context without specify the fourth arguments to false, it's going to create that Mediator automatically also, that two things if it count.

    You're right, It's should be considered as sugar, which need to be trade off at the end, that's the reason I've not open any pull request yet, it's bothers myself too...

  5. Support Staff 5 Posted by Stray on 11 May, 2011 08:51 AM

    Stray's Avatar

    Nice to think about these things though! It's better to ask ourselves a question, even if we find, after exploring it, that the answer is 'no', than to just assume that we already know the answers :)

    Stray

  6. Stray closed this discussion on 11 May, 2011 08:51 AM.

Comments are currently closed for this discussion. You can start a new one.

Keyboard shortcuts

Generic

? Show this help
ESC Blurs the current field

Comment Form

r Focus the comment reply box
^ + ↩ Submit the comment

You can use Command ⌘ instead of Control ^ on Mac