3
\$\begingroup\$

I am writing a JavaScript/HTML driven web application. For the user interface, I am not completely sure that my JavaScript is 'OK'. Mainly, I switch between "modes" by assigning a placeholder variable to a new function, which changes what the onclick event does for items in a table (either open, edit, or move them).

Am I breaking any code styles? Is my script hard to follow or understand? Is it unconventional for me to assign bookmarkEventMapping to another function later on in my code, using it as a placeholder? What can I do differently? Can you recommend any material that I could read to write clearer JavaScript?

(Please note that this is a WIP)

var bookmarkEventMapping = function() {};//placeholder
/*
    ...
    more JavaScript, which creates <li> elements
    that all have onclick events, example:
        element.addEventListener(
            "click",
            function(e) { bookmarkEventMapping(e); }
        );
    ...
*/
function initModeMenu() {//called in <body> onload event
    var modeNames = ["normal","edit","move"];//the order in HTML menu
    for(var i = 0; i &lt; 3; i++) {,<!-- note: escaped &lt; in loop -->
        var node = document.getElementById("modeMenu").children[i];
        bookmarkActions[ modeNames[i] ].node = node;
        node.dataset["action"] = modeNames[i];
        node.addEventListener(
            "click",
            function() {
                bookmarkActions.setCurrentMode( this.dataset["action"] );
            }
        );
    }
    bookmarkActions.setCurrentMode("normal");
}
var bookmarkActions = {
    "currentMode":"normal",
    "setCurrentMode":function(modeName) {
        var prevMode = bookmarkActions.currentMode;
        bookmarkActions[ prevMode ].exitMode();
        bookmarkActions[ prevMode ].node.classList.remove("selected");
        bookmarkEventMapping = bookmarkActions[ modeName ].eventMapping;
        bookmarkActions[ modeName ].node.classList.add("selected");
        bookmarkActions.currentMode = modeName;
    },
    "normal":{
        "initMode":function() {
        },
        "exitMode":function() {
        },
        "eventMapping":function(ev) {
        }
    },
    "edit":{
        "initMode":function() {
        },
        "exitMode":function() {
        },
        "eventMapping":function(ev) {
            if(ev.type == "click")
            {
                bookmarkActions.edit.selectBookmark( ev.target );
            }
        },
        "selectBookmark":function(itemNode) {
            itemNode.dataset['bookmarkItemId'];
            itemNode.classList.add("editing");//.remove later
        }
    },
    "move":{
        "initMode":function() {
        },
        "exitMode":function() {
        },
        "eventMapping":function(ev) {
        }
    }
};

Below is a relevant snippet of HTML from the <body>

<menu id="modeMenu">
    <li>Normal</li>
    <li>Edit</li>
    <li>Move</li>
</menu>
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Readability - the code reads fine. The modes concept is explained well just by reading the code, so I wouldn't worry about that.

Code style - Javascript programmers are used to passing functions around and probably wouldn't bat an eyelid at the idea of assigning different functions to a variable, but it makes more sense to me to always have the same function as the event handler, and call the action for the current mode from within it:

var bookmarkEventMapping = function(e) {
    bookmarkActions[bookmarkActions.currentMode].eventMapping(e);
};

A couple of other things I would do, out of personal preference more than anything:

  • store the list of modes separately, not in the bookmarkActions object
  • refer to the modes using references to the actual objects, as opposed to using the object property names.

If you did store the mode list separately, you could also make things cleaner by making that list the only place where information about the modes is stored and using it to generate the html, instead of having to maintain the html and the code in initModeMenu separately.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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