How would you write this ActionScript code?

I was just watching Lee Brimelow’s fantastic video about Flash Catalyst and Flash Builder and noticed some Flex / ActionScript code (generated by Flash Catalyst) that caught my attention:

var state:String = currentState;
if ( state == 'closed' ) {
    currentState='open';
}
if ( state == 'open' ) {
    currentState='closed';
}

This code just toggles the currentState property between ‘open’ and ‘closed’. I initially thought that the code was not very elegant in that form but that maybe there was a reason it was written like that. Maybe Flash Catalyst needs it in that form to understand it. Maybe it’s more efficient for the VM to run. I’m not totally sure. But it made me curious… How would you write the same functionality? Use a ternary operator? A switch statement? If – else if? If – else? And why would you do it that way? Is it your personal preference, are there performance implications, SWF size implications, is your way more readable, or are there other factors? While programming provides us many ways to express our creative freedom it seems that for common patterns like this there might be a universal “best way” to do this. Or perhaps this is just another tabs versus spaces kind of issue. I’d love to hear what the community thinks!

  • I’d prefer a ternary statement. I don’t like taking up multiple lines for something so simple.

    As far as referencing the currentState value first, this would be faster if the currentState getter is heavy. Not sure.

    Even if this is faster for some reason, the second condition should at least be in an else..if statement so it doesn’t run the condition if the first statement is true.

    Personally, I’d consider this poor code.

  • if ‘open’ and ‘closed’ are the only two possible states, easily:
    var isOpen:Boolean;
    ….
    isOpen = !isOpen;

  • I get the impression the code is written that way since those states were probably added sequentially, and there’s no way to know how many conditions you’re going end up with, so the compiler won’t know to combine them, or what not.

    I suppose you could have an optimization process that would go through and clean up the code, but that would take an awful lot of processing power, and you wouldn’t want the compiler making decisions for you when it’s just as easy to leave it alone.

    Personally, I like to use ternary when I can, and switch statements rather than long if/else.

    iBrent

  • Hi iBrent,

    I think you are right as to why the code was written that way by Flash Catalyst. I was thinking that it’s obviously a two option toggle but to the tool it’s two distinctly different and disconnected condition checks. It’s funny how tools and coders think differently.

    BTW: I also prefer the ternary operator for a two option toggle. For more than two options I’ve always used if – else if. The only reason I can think of as to why is all those breaks you usually have to put in a switch block in AS3. But maybe I should give switch a try again.

    -James

  • JW

    I’m with Danny…This code looks broken to me.

    If state is ‘closed’ on entry, it will still be ‘closed’ after this code executes. It doesn’t toggle in this case. Am I missing something?

  • JW

    …adding… Never mind, I see what I was missing.

  • Hi JW,

    I thought this exact same thing at first. But the code does actually work since the change is made to currentState not to state. Kinda tricky from a readability perspective.

    -James

  • Looking at the option I would probably go switch, since the possibility for multiple states is greatly increased. If I know the option is only going to be two, sure I would use ternary but the odds are that this will become 3+ so I like the elegance and easy readability of switches. There is also a likely chance that I will want to add additional functionality beyond just setting state and ternary doesn’t handle that as well. Finally, I can easily order my switch in priority order, the most likely at top, etc. without having to re-cuddle my curlies…

  • Keith Thomas

    I still not have learnt how to use this technology :-( However, this will not stop me commenting as commenting on something about which I know nothing is one of my favorite hobbies.

    Anyway, if this technology supports OO concepts – may god bless your souls if it does not – then to manage state I would use the state pattern (GoF). Duh.

  • interesting

    …maybe the program is considering the probability that someone might change the value of ‘state’ to ‘open’ within that if statement.

    :P

  • BTW: I also prefer the ternary operator for a two option toggle. For more than two options I’ve always used if – else if. The only reason I can think of as to why is all those breaks you usually have to put in a switch block in AS3. But maybe I should give switch a try again.

    I agree completely James. I would say though that even though a switch block may be longer, because of the breaks, they are usually much easier to read than a series if/else statements. Depending on the developer’s stryle, you might have to count the if statement’s closing bracket as a line too. :)

    -TH

  • Fredy

    I prefer ternary operator.
    But when I’m sure that just have two states, I create a variable to keep the state, like ‘yeah’ said.

    var isOpen:Boolean = false;

    isOpen = !isOpen;

  • Dev

    If there are only two states,

    currentState = ( state == ‘closed’ )? ‘open’ : ‘closed’;

  • I would do it the way iBrent said. if… else for two options, but then use switch for more. I use to use if… else if, but but moved away from it for some unknown reason – maybe readability.

  • currentState = currentState == “open”
    ? “closed”
    : “open”;

  • radekg

    currentState = ( currentState == “closed” ) ? “open” : “closed”;

  • Adam

    A cleaner technique i learned from python is to do something like:

    static var TOGGLE_STATE = {
    ‘open’: ‘closed’,
    ‘closed’: ‘open’
    };

    state = TOGGLE_STATE[state];

  • I see several things wrong with this.

    First, the two if statements aren’t connected. If you replaced the second if block with an else block hooked to the first if, then the code wouldn’t have to evaluate the variable twice.

    Second, it would be a lot cleaner to use a ternary operator here, since you’re only working with two possibilities you wouldn’t sacrifice clarity.

    Third, and what bothers me the most, is the use of a completely unnecessary new variable, state. (although seeing this block in context might reveal otherwise) It looks like the programmer was using state to hold the value of currentState in order to provide a variable to evaluate while changing currentState. If the programmer had only used an else block instead of another if block, this wouldn’t be necessary at all.

    And finally, if currentState can only switch between ‘open’ and ‘closed’ then it’s wasteful to use a string. The programmer should have used a boolean instead and called the variable isOpen or something like that.

    So ideally, it would be reduced to:

    isOpen = !isOpen;

    If pulling a string value (‘open’ or ‘closed’) from this is necessary, then a ternary should be used in the code that uses the string, e.g.:

    txt.text = “The state is ” + (isOpen)?”open”:”closed”;

    If the string value is used multiple places in the code, and the variable scope is over the entire class, the pair can be written like:

    public var isOpen:Boolean = true;
    public function get currentState():String { return (isOpen)?”open”:”closed”; }

  • Good insight David. I’ll add another thing to your list… Instead of using mutable strings for “open” and “closed” those should probably be consts.

    -James

  • Daniel Larsen

    Wow, David. That is genius! I love tricks like that. Thanks for sharing.

  • Daniel Larsen

    oops, I meant Adam. :-P

    (not to discount David’s ideas…but those are the same ones that I had…Adams idea was totally new to me)

  • Matt Cannizzaro

    Hi James,

    I’m the developer working on interactions in Flash Catalyst. Most programmers, myself included, would probably write something similar to the following to toggle state between “open” and “closed”:

    currentState = ( currentState == “closed” ) ? “open” : “closed”;

    This code is simple, straightforward, and readable. Let me explain why our generated code ends up being more complex:

    (For those of you who have not used Flash Catalyst, a brief explanation: an interaction is just a snippet of Actionscript code that either sets the state of a component or plays an effect. It can have a condition attached that causes it to only execute when a component is in a given state).

    Interactions in FC are not mutually exclusive – if I have several interactions that fire when a user clicks a button, any number of them could end up executing. As a result, we can’t just use an if/else block, or our interactions would end up being mutually exclusive.

    When only a single component and a single property (currentState in this example) are involved, an if/else block would indeed be the most appropriate structure, but that is a special case. For example, consider the following interactions:

    On click, play transition to state1, when in state2.
    On click, play transition to fooState in someComponent, when someComponent is in barState.

    There are two components involved here: the component you’re editing, and someComponent, a child component. We might be tempted to write these interactions like so:

    if (currentState == “state2”) {
    currentState = “state1”;
    } else if (someComponent.currentState == “barState”) {
    someComponent.currentState = “fooState”;
    }

    But if we wrote that, we’d have a bug: when our component is in state2, we never execute the second interaction, regardless of the state of someComponent. The correct code, for this case, looks like:

    if (currentState == “state2”) {
    currentState = “state1”;
    }
    if (someComponent.currentState == “barState”) {
    someComponent.currentState = “fooState”;
    }

    In the general case, we have to use independent if blocks. This makes sense because interactions in FC are fundamentally independent from one another and the code should reflect this.

    Now let’s consider your example, where we want to toggle between open and closed states. Knowing that we should use separate if blocks, we might come up with:

    if (currentState == “open”) {
    currentState = “closed”;
    }
    if (currentState == “closed”) {
    currentState = “open”;
    }

    Again, we have a bug: if currentState is “open” before this code runs, it will still be “open” after it runs, because both conditions evaluate to true. The problem here is that we’ve written code that assumes the value of the currentState property won’t change until it has finished executing, which is of course a flawed assumption.

    Considering that we’ve already determined that if/else doesn’t work in some cases, the general solution is to do what FC does: store the value of currentState before we’ve changed anything and test against this saved value. When an FC user writes interactions, they are probably not considering that the value of their condition might change part way through the handling of a single event. The generated code is probably not what a programmer would write, but it most accurately reflects the intentions of the FC user.

    Certainly in this example, this code looks silly. In the general case, however, using the extra variable is necessary. I decided that it was best to keep our code generation consistent, even if that means making the simple cases look slightly more complex than they actually are.

    – Matt

  • Thanks Matt for the explanation. With those constraints it seems that the way you’ve chosen is the best way. It been interesting for me to learn why different people choose different ways depending on their own constraints.

    Perhaps the only thing that Catalyst should change is to use consts for the state names instead of strings. Maybe you could even bind to those consts in the State tags. I remember hearing at one point that there might be some compiler optimizations done when binding to consts. If so it would be nice to not have state names as strings everywhere.

    -James

  • Matt Cannizzaro

    Hi James,

    It would be nice if we could use a single constant for a string name. Typically this is done so that should you ever want to change the value of the string, you need only change it in one place. We would miss out on this benefit because AFAIK you cannot use anything but a literal string in a State tag, and state specific properties are always of the form stateName.propertyName. So even if we were to use constants in the generated Actionscript code, if you wanted to rename a state, you’d still have do something like a search and replace.

    – Matt

  • @Adam
    I like the Python-isk ideas. However, when I tried to implement it in Flex, I ran into a brick wall. Is there a way to make the following function cleaner in AS?

    private function getQuarterDescription( quarter:int ):String
    {
    switch(quarter)
    {
    case 1:
    return “1st”;
    case 2:
    return “2nd”;
    case 3:
    return “3rd”;
    case 4:
    return “4th”;
    }

    return “”;
    }

  • I suppose this would be cleaner for that particular example:

    private var quarterDesriptions:Array = [ “”, “1st Quarter”, “2nd Quarter”, “3rd Quarter”, “4th Quarter” ];

    quarterDesription = quarterDesriptions[quarter];

    But, when comparing ternary, if/else or switch, I’m wondering how you would set this it up like Python.

    -TH

  • Hi Matt,

    Don’t you get compile errors when you reference an non-existant state in the new state syntax in an MXML tag? The const doesn’t help avoid state name typos in MXML but should help to keep typos in ActionScript from causing problems that are not picked up by the compiler.

    -James

  • Hi James,
    I go with yeah.
    Use a boolean if it’s enough, which seems to be the case.
    Ariel

  • Rob

    Agree with yeah and Matt Cannizzaro – if you just need a toggle, use a boolean; and if you need more conditions use a switch. Either way you should be able to avoid using the extra variable (which should be a const for performance) – definitely easier on memory and the GC.

  • Matt Cannizzaro

    James,
    If you had a constant for each state name and made sure to always use the constant, never a string literal, then you would avoid typos that would cause runtime errors. Using constants for state names is not really a common idiom in Actionscript though – if you look at the source of the Spark classes in the Flex 4 SDK, for example, you’ll see they use literal strings for state names, not constants.

    Ariel, Rob,
    Neither booleans nor switch statements are suitable in this context, though they may appear more appropriate than the actual generated code at first glance. Please see my above post for the details. Remember that we are trying to generate code that works in the general case, not just this particularly simple case.

    While the variable could (and perhaps, *should*) be a const, I think the primary justification would be readability, not performance. Making it clear that we don’t intend to change the value of that variable might help make this code clearer, which is of course a good thing. But this code is not executed often, so any performance benefit derived from using a const would be negligible.