HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Is there anything wrong with this double ternary?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisanythingwithternarydoublewrongthere

Problem

Is there anything wrong with using double ternary operator like so:

var stateName:String = state is String ? state as String : "name" in state ? state.name : null;


Here is two lines because someone requested it:

var stateName:String = state is String ? state as String : 
                        "name" in state ? state.name : 
                            null;


Here is the alternative:

if (state is String) {
    stateName = state as String;
}
else if ("name" in state) {
    stateName = state.name;
}
else {
    stateName = null;
}


The function:

/**
 * Sets the style in the state specified. Creates state if it doesn't exist.
 * */
public static function setStyleInState(styleName:String, value:*, state:*, target:UIComponent = null):void {
    var stateName:String = state is String ? state as String : "name" in state ? state.name : null;

    if (!StateUtils.hasState(target, state)) {
        state = createState(stateName, null, target);
    }
    else if (!(state is State)) {
        state = getState(target, stateName);
    }

    // more code to write
}


It makes sense to me but I'm the one writing it.

This is ActionScript3 or ECMAScript 4 (basically JavaScript with strong typing).

Solution

Given your example I think that your double ternary operation should actually be a separate function. When you start getting complex one-liners like this, it is a sign that you need another function. Here's how I approached it:

function getStateName(state) {
  if(state is String) {
    return state as String;
  }

  // This will always return "null" if state.name does not exist.
  // I don't know about AS, but in JavaScript if something doesn't
  // exist it returns as undefined (not null), but you want the null
  // return. As far as I am aware, AS supports the || short-circuit and
  // truthy/falsey equivalence, like JavaScript.
  return state.name || null;
}


This could be used in your function like so;

public static function setStyleInState(styleName:String, value:*, state:*, target:UIComponent = null):void {
    var stateName:String = getStateName(state);

    if (!StateUtils.hasState(target, state)) {
        state = createState(stateName, null, target);
    }
    else if (!(state is State)) {
        state = getState(target, stateName);
    }
}


There's more to be said here; I think that exposing mutability like this on a static method (instead of on the instance of a state) is going to lead you into trouble later on down the road and you should probably re-evaluate your design.

Specifically, state in static methods is always a recipe for disaster when it comes to testability and debugging, especially when you branch into multiple threads.

Code Snippets

function getStateName(state) {
  if(state is String) {
    return state as String;
  }

  // This will always return "null" if state.name does not exist.
  // I don't know about AS, but in JavaScript if something doesn't
  // exist it returns as undefined (not null), but you want the null
  // return. As far as I am aware, AS supports the || short-circuit and
  // truthy/falsey equivalence, like JavaScript.
  return state.name || null;
}
public static function setStyleInState(styleName:String, value:*, state:*, target:UIComponent = null):void {
    var stateName:String = getStateName(state);

    if (!StateUtils.hasState(target, state)) {
        state = createState(stateName, null, target);
    }
    else if (!(state is State)) {
        state = getState(target, stateName);
    }
}

Context

StackExchange Code Review Q#111007, answer score: 5

Revisions (0)

No revisions yet.