26
votes
\$\begingroup\$

I've recently been doing some mods to some old code I've been maintaining for a couple of years now.

As part of a wider set of scripts using YAHOO YUI 2.2 (yes, that old) for dialog-style panels, I have a function that listens to click events on 3 buttons in a given panel set:

addFooListeners = function (panelType) {

    YAHOO.util.Event.addListener("show" + panelType, "click", showFoo, eval(ns + ".panel_" + panelType), true);
    YAHOO.util.Event.addListener("hide" + panelType, "click", hideFoo, eval(ns + ".panel_" + panelType), true);
    YAHOO.util.Event.addListener("commit" + panelType, "click", commitFoo, eval(ns + ".panel_" + panelType), true);

}

This code is the result of a number of panels/dialogs having near identical behaviour.

For this round of development I've needed to add the ability to do things a little differently from time to time, so I added an object, overrides which allows me to point the show, hide and commit actions elsewhere. I came up with the following:

addFooListeners = function (panelType, overrides) {

    var handlers = {
        show: ( overrides != null ? ( overrides.show != null ? overrides.show : showFoo ) : showFoo ),
        hide: ( overrides != null ? ( overrides.hide != null ? overrides.hide : hideFoo ) : hideFoo ),
        commit: ( overrides != null ? ( overrides.commit != null ? overrides.commit : commitFoo ) : commitFoo )
    }

    YAHOO.util.Event.addListener("show" + panelType, "click", handlers.show, eval(ns + ".panel_" + panelType), true);
    YAHOO.util.Event.addListener("hide" + panelType, "click", handlers.hide, eval(ns + ".panel_" + panelType), true);
    YAHOO.util.Event.addListener("commit" + panelType, "click", handlers.commit, eval(ns + ".panel_" + panelType), true);
}

As you can see I'm enforcing default behaviour unless an appropriate attribute in the overrides object is set with another function (named or anonymous).

Is there a cleaner / more readable way to concisely setup the handlers object? The nested ternary seems to my eyes to be cluttering things a bit but other approaches like if ( overrides != null ) { ... } seem just as messy.

\$\endgroup\$
  • 1
    \$\begingroup\$ My personal opinion is that the ? ternary operator is worth using only when it's not nested. In your case it just makes the code more unreadable and unmaintainable. \$\endgroup\$ – UncleLaz Jan 20 '12 at 21:47
  • \$\begingroup\$ I like formatting nested ternary operators so they read like multiple questions: codereview.stackexchange.com/a/6562/8891 \$\endgroup\$ – seand Jan 20 '12 at 22:14
28
votes
\$\begingroup\$

Let's do some fixing. First, this is how you pass optional (non-boolean) parameters in JS (the Good Waytm):

addFooListeners = function (panelType, handlers) {
    handlers        = handlers        || {};
    handlers.show   = handlers.show   || showFoo;
    handlers.hide   = handlers.hide   || hideFoo;
    handlers.commit = handlers.commit || commitFoo;

The above can be rewritten in a neater way using jQuery (not sure what the name of YUI equivalent to extend is):

handlers = $.extend({
    show  : showFoo, 
    hide  : hideFoo, 
    commit: commitFoo
}, handlers || {})

Now, using eval for this code is criminal. Say the object ns refers to is module, then you can do this instead of eval:

YAHOO.util.Event.addListener("show" + panelType, "click", handlers.show, module["panel_" + panelType], true);
YAHOO.util.Event.addListener("hide" + panelType, "click", handlers.hide, module["panel_" + panelType], true);
YAHOO.util.Event.addListener("commit" + panelType, "click", handlers.commit, module["panel_" + panelType], true);

Now, as you can see, you are assigning a lot of events in a similar fashion. Did you think of defining an addPanelListener function within your function?

function addPanelListener (event, panelType, handler) {
    YAHOO.util.Event.addListener(event + panelType, "click", handler, module["panel_" + panelType], true);
} 

addPanelListener("show"  , panelType, handlers.show);
addPanelListener("hide"  , panelType, handlers.hide);
addPanelListener("commit", panelType, handlers.commit):

Hope it helps.

\$\endgroup\$
  • 9
    \$\begingroup\$ +1 for flagging use of eval as criminal :) I find "the Good Way" more readable than the jQuery way. \$\endgroup\$ – Eric Bréchemier Jan 31 '11 at 13:06
  • 6
    \$\begingroup\$ Using eval in my company pretty much guarantees a 3-hour basics of javascript meeting with me :) \$\endgroup\$ – glebm Jan 31 '11 at 21:25
  • \$\begingroup\$ aaah good old "the-first-one-is-free" eval. Much like a drug pusher. I cringe every time I see that in my code (especially the code I post here ;-). \$\endgroup\$ – LRE Jan 31 '11 at 21:32
  • \$\begingroup\$ @glebm can you elaborate on what makes "The Good Way" the good way? \$\endgroup\$ – LRE Jan 31 '11 at 21:34
  • 2
    \$\begingroup\$ Of course. It is easily readable, less error-prone, and, as a bonus, it's faster. :) \$\endgroup\$ – glebm Jan 31 '11 at 21:36
7
votes
\$\begingroup\$

It looks like you can reduce that ternary a bit by using && like this:

var handlers = {
    show:  ( overrides != null && overrides.show != null ? overrides.show : showFoo ),
    hide:  ( overrides != null && overrides.hide != null ? overrides.hide : hideFoo ),
    commit: ( overrides != null && overrides.commit != null ? overrides.commit : commitFoo )
}

I'm not too familiar with javascript but does the function parameter have to be checked against null? Like for example, can you further shorten the check by doing something like this?

show:  ( overrides && overrides.show ? overrides.show : showFoo ),
hide:  ( overrides && overrides.hide ? overrides.hide : hideFoo ),
// ...
\$\endgroup\$
  • 2
    \$\begingroup\$ Generally speaking, I think you're right about not needing to check against null - however it can be not-null and false then it would evaluate to false (though in this case I think that'd be ok) \$\endgroup\$ – LRE Jan 31 '11 at 0:03
  • \$\begingroup\$ Overrides.show looks like it could be a boolean; if so, you'd need to test it against null explicitly. \$\endgroup\$ – Fred Nurk Jan 31 '11 at 1:36
  • \$\begingroup\$ @Fred the intention is to pass functions for show, hide and commit. If a false is passed there then showFoo() will be used. If a true is passed there then that'll be passed in place of showFoo() and cause trouble further down so I guess I need to tread carefully there. Having said that though, I'll only ever be passing a function in those parameters so I'll probably survive for now. \$\endgroup\$ – LRE Jan 31 '11 at 1:48
  • \$\begingroup\$ Might be worth pointing out that this use of && is only valid as long as && is a short-circuiting and operation (which is a safe assumption for C-derived languages, but not all languages). \$\endgroup\$ – Todd Lehman Jan 22 '12 at 8:01
  • 1
    \$\begingroup\$ Can be even shorter: show: overrides && overrides.show || showFoo \$\endgroup\$ – some Aug 13 '12 at 22:51
3
votes
\$\begingroup\$

In JavaScript, you are not required to explicitly check if a variable is null or undefined because:

  1. null or undefined return false in a boolean expression.
  2. JS expressions are evaluated from left to right. So for a || b, if a is false, then only b will be evaluated. But if a is true, b will not be checked. Similarly for a && b if a is false, b will not be evaluated.

Hence, if (a != null) { "do something" } can be written as if (a) { "do something" } or simply a && "do something".

In the same way it can be used to set a default value when the value is set to null or undefined:

function someFunction(age){
    :
    var age= age|| 18;
    :
}

someFunction(28) results in 28 whereas someFunction() results in 18.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.