0

In below code, I pass an HMTL element and check whether parameter passed is null or not using ternary operator. If it is not null, I change the className of the passed element.

var changeColorTo = {

  grey: function(e){
    e ? (e.className = "grey") : "" ;
  },
  red: function(e){
    e ? (e.className = "red") : "" ;
  },
  green: function(e){
    e ? (e.className = "green") : "" ;
  },
  blue: function(e){
    e ? (e.className = "blue") : "" ;
  }
};

The above code works fine except when I pass any random string like

changeColorTo.grey("random");

It doesn't cause any harm. But I am wondering is above code correct? Do I miss anything? or is there any better way to achieve the same result?

Thank you.

indusBull
  • 1,834
  • 5
  • 27
  • 39
  • If you want to be short and concise you could write `e && e.className && (e.className = "grey");` with no ternary at all. – david Nov 02 '11 at 23:09
  • If by "shorter" you mean less code, then it isn't—43 characters vs 32 in the OP. And less code is not, of itself, a good reason to do anything. – RobG Nov 02 '11 at 23:27

2 Answers2

3

You could expand your condition from just e to (e && e.className). That should prevent script errors resulting from passing in random junk or even non-element nodes.

Better, implement that condition as function hasClassName(e) { return ... } and use hasClassName(e) as your test.

EDIT: Replaced less-than-fully-compatible (typeof e=="object") && ('className' in e) condition, per comments. See also How do I check if an object has a property in JavaScript?

Community
  • 1
  • 1
David O'Riva
  • 696
  • 3
  • 5
  • If *e* is a host object, there is no reason to believe that it will return "object" for a typeof test (host objects are not require to conform to ECMA-262 and in many cases don't). So the above test is likely more error prone than something like `if (e && e.className)...`, provided *e* already has a *className* property. – RobG Nov 02 '11 at 23:31
  • Interesting. I haven't encountered non-"object" host objects before, but we've been highly focused on just the popular browsers for PC/Mac. But couldn't they also, then, decide to throw an exception on the member lookup, or return a value that explodes in a boolean test? – David O'Riva Nov 03 '11 at 00:15
  • Misbehaving host objects are common, those implemented using ActiveX in IE are notorious (or infamous) for it. Though browsers are starting to standardise on JavaScript™, consistency with ECMA-262 and implement prototype inheritance for DOM objects, the intention of the DOM is to be language neutral, so expecting host objects to implement behaviour and features of one particluar language is not in the spirit of the web (though HTML5 seems to have dispensed with that to a large extent). – RobG Nov 03 '11 at 01:45
  • @RobG I'm still learning Javascript (Reading JavaScript: The Definitive Guide). Pardon me if it's really basic question but what is "host object"? I see SO thread [How do I learn about browser host objects] (http://stackoverflow.com/questions/6764044/how-do-i-learn-about-browser-host-objects) but there is no clear answer as questionnaire expected. – indusBull Nov 03 '11 at 18:35
  • 1
    A host object is anything provided by the host environment (such as DOM elements). Strictly, there are built-in objects, which are those provided by ECMAScript (Object, Array, Math and so on), then there are native objects, which are any object built using ECMAScript (e.g. `var obj = {}` or `function foo(){}`), everything else must be provided by the host environment so is a host object (window, document, XMLHttpRequest, etc.). – RobG Nov 03 '11 at 21:00
  • @RobG Thank you for simple explanation – indusBull Nov 03 '11 at 21:07
1

The code as it stands, will work if you pass in a string. However, if you want to be sure that you're only passing in a DOM element (it's better to be strict), you can modify your code to this:

function isNode(o){
  return (
    typeof Node === "object" ? o instanceof Node : 
    typeof o === "object" && typeof o.nodeType === "number" && typeof o.nodeName==="string"
  );
}    

function isElement(o){
  return (
    typeof HTMLElement === "object" ? o instanceof HTMLElement : //DOM2
    typeof o === "object" && o.nodeType === 1 && typeof o.nodeName==="string"
  );
}

var changeColorTo = {
    grey: function(e) {
        isNode(e) || isElement(e) ? (e.className = "grey") : "" ;
    },
    ...
}

For more information on how isNode and isElement work, take a look at this stackoverflow answer. This code will also ensure that you won't try to change the className attribute of a null or undefined variable since the first condition in each of those functions (o instanceof Node and o instanceof HTMLElement) will fail, which ensures that isNode and isElement will return false for null and undefined values.

Community
  • 1
  • 1
Vivin Paliath
  • 94,126
  • 40
  • 223
  • 295
  • @RobG Do you have specific examples of which browsers in which this would fail? I've used this before (see the linked answer) and it has worked on the major browsers (FF, Chrome, IE, Opera) without issues. – Vivin Paliath Nov 02 '11 at 23:51
  • I'm still trying to decipher the above functions. It's little complicated for me. But my initial test shows that, Chrome returns false for (**typeof Node === "object"** and **typeof HTMLElement === "object")** both functions. But I've no idea why it is so. It works for my problem but does more than what I want. RobG's answer is really short. Thanks – indusBull Nov 03 '11 at 18:42
  • Both isNode and isElement work in chrome. What I meant that in Chrome (typeof HTMLElement === "object") returns false which means second choice (typeof o === "object" && o.nodeType === 1 && typeof o.nodeName==="string") is evaluated which does return true. It's just that I don't understand why it fails on Chrome. Same for isNode. – indusBull Nov 03 '11 at 20:12
  • `typeof Node` and `typeof HTMLElement` both return `function` in Chrome, which is why the second part is there to make sure that the element really is a DOM element. – Vivin Paliath Nov 03 '11 at 20:44