1
\$\begingroup\$

Please review the code.

document.addEventListener("keydown", function(e) { // shortcuts
  if (e.ctrlKey) { // Ctrl+
    if (/^(82|79|83|66|191)$/.test(e.keyCode)) {
      e.preventDefault();
    }
    switch (e.keyCode) {
      case 82: // R
        newDoc();
        break;
      case 79: // O
        openDoc();
        break;
      case 83: // S
        saveDoc();
        break;
      case 66: // B
        showHideStatusBar(statusBarOn ? false : true); // toggle
        break;
      case 191: // /
        alert("Welcome to " + appname + "!");
        break;
    }
  }
  if (e.keyCode == 9) { // tab
    e.preventDefault();
    var sStart = textarea.selectionStart,
      text = textarea.value;
    textarea.value = text.substring(0, sStart) + "\t" + text.substring(textarea.selectionEnd);
    textarea.selectionEnd = sStart + 1;
  }
});
\$\endgroup\$
  • \$\begingroup\$ What's the background/usage of your app? On what basis you want to review your code? readability/clean-code/optimization? Please mention them. \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 10 '13 at 11:14
  • \$\begingroup\$ optimization and simplifying mainly... it is for a text-editing webapp.. \$\endgroup\$ – user31946 Nov 10 '13 at 11:17
3
\$\begingroup\$

In JS, when you have a switch, it's usually better to go with a dispatch table.

var mapping = {
    82: newDoc,
    79: openDoc,
    83: saveDoc,
    66: function() {
        showHideStatusBar(statusBarOn ? false : true);
    },
    191: function() {
        alert("Welcome to " + appname + "!");
    }
};
mapping[e.keyCode]();

Or if you don't want to give a name:

({
    82: newDoc,
    79: openDoc,
    83: saveDoc,
    66: function() {
        showHideStatusBar(statusBarOn ? false : true);
    },
    191: function() {
        alert("Welcome to " + appname + "!");
    }
})[e.keyCode]();

Also, use === instead of == for multiple reasons. Google "triple equal javascript" if you want more information.

\$\endgroup\$
  • \$\begingroup\$ It's generally a good idea to use a dispatch table instead of a switch. However, you have to handle a default case during lookup, something like var fn = mapping[e.keyCode]; if (fn !== undefined) { fn() } else { default() } \$\endgroup\$ – amon Nov 10 '13 at 11:03
  • \$\begingroup\$ There is no default case in his code. \$\endgroup\$ – Florian Margaine Nov 10 '13 at 13:35
  • \$\begingroup\$ Except there is an if preventing this :) \$\endgroup\$ – Florian Margaine Nov 10 '13 at 15:01
  • \$\begingroup\$ /me crawls back in shame for not reading the code carefully… thanks. \$\endgroup\$ – amon Nov 10 '13 at 15:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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