2
\$\begingroup\$
  • I am working on a Python/Django project which has turned into a JavaScript/jQuery project due to the amount of asynchronous functionality that I thought to add.
  • There is a JavaScript file which is doing everything for the JS side. I am a newbie in JavaScript/jQuery and am starting to think that this is going to become a big problem soon.
  • I asked for a previous review but as I never got one there and the file has changed a lot since then I am placing part of the file where I think something can be be improved.

Some functions which are being used by the code:

function bind_events(cur_obj, e) {
    var trigger_name = "";
    if (e.keyCode == 13) {
        trigger_name = EV_ENTER_KEY;
    } else if (e.altKey && e.keyCode == "N".charCodeAt(0)) {
        trigger_name = EV_ALT_N;
    }
    $(cur_obj).trigger(trigger_name);
}

//Auxiliary functions for categories
function get_all_categories() {
    var class_ids = [];
    $(CLASS_DEL_CATEGORY).each(function () {
        class_ids.push(get_class_string(this, 11));
    });
    return class_ids.join(",");
}

function enter_category(category_box, trigger_event, trigger_data) {
    if (value_in_selector(category_box, CLASS_UI_MENU_ITEM) == false) {
        alert(MSG_NOT_CATEGORY);
    } else if (value_in_selector(category_box, CLASS_CATEGORY) == true) {
        alert(MSG_CATEGORY_ADDED);
        category_box.val("");
    } else {
        category_box.trigger(trigger_event, trigger_data);
    }
}

function add_category(cur_obj, e, data) {
    $.ajax({
        url: URL_CATEGORY,
        data: {'value': $(cur_obj).val()},
        success: function (output) {
            data.cat_list.append(output);
            $(cur_obj).val("");
            if (data.trigger_obj !== undefined) {
                data.trigger_obj.trigger(data.next_trig);
            }
        }
    });
}

The code that I think needs to be improved is below.

//Actions for Search's category box
CATEGORY_INPUT.on({
    keyup: function (e) {
        bind_events(this, e);
    },

    "enterKey": function () {
        enter_category($(this), EV_ADD_CATEGORY, [
            {
                cat_list: CATEGORY_LIST_SEARCH,
                trigger_obj: BOOKMARK_LIST,
                next_trig: EV_UPDATE_BOOKMARKS
            }
        ])
    },

    "addCategory": function (e, data) {
        add_category(this, e, data);
    },

    "clearAll": function () {
        $(CLASS_CATEGORY).remove();
        $(this).val("");
        BOOKMARK_LIST.trigger(EV_CLEAR_ALL);
    }
});

//Actions for Add Bookmark's category box
CATEGORY_BOX.on({
    keyup: function (e) {
        bind_events(this, e);
    },

    "enterKey": function () {
        enter_category($(this), EV_ADD_CATEGORY, [
            {
                cat_list: CATEGORY_LIST_ADD,
                trigger_obj: undefined,
                next_trig: undefined
            }
        ])
    },

    "addCategory": function (e, data) {
        add_category(this, e, data);
    }
});

As you can see nearly same events are bound to both with minor changes. I thought to bind events to multiple selectors but as not all are same events readability will suffer.

Any thoughts for improvements?

\$\endgroup\$
  • \$\begingroup\$ What are the values of EV_ADD_CATEGORY and EV_ENTER_KEY? I see those constants used in some places, but - I suspect - not all the places they ought to be. \$\endgroup\$ – Flambino Jun 8 '14 at 9:28
  • \$\begingroup\$ @Flambino These are strings representing the events that I wrote to avoid hard-coding at many places but I came across a problem stackoverflow.com/questions/24095288/… due to which I had to hard-code the values. Not happy about it but have been unable to find a workaround. \$\endgroup\$ – Aseem Bansal Jun 8 '14 at 9:36
2
\$\begingroup\$

Structurally, it doesn't look too bad, although it seems inconsistent.

By convention, JavaScript uses camelCase naming - not snake_case. All your code is snake_case, but then you have things like keyCode and charCodeAt because that's what JS's own APIs use. So I'd encourage you to switch you code to camelCase in keeping with JS conventions.

You've got a number of "constants" (in quotes, since JS doesn't actually have constants), like EV_ENTER_KEY, which, judging from your comment, is a string (enterKey in this case). Sometimes you use the constants by name, other times you use them by value.

While it's true you can't do this:

CONST = "foo";
obj = {
  CONST: "bar"
};

and get an object like {foo: "bar"} (because JS interprets the CONST: literally), you can do this:

obj = {}
obj[CONST] = "bar";

And your object will be {foo: "bar"}.

So that would clean things up, and allow you to actually use constants. E.g.

categoryBoxEvents = {}
categoryBoxEvents.keyup = function (e) {
  bind_events(this, e);
};

categoryBoxEvents[EV_ENTER_KEY] = function () {
  enter_category($(this), EV_ADD_CATEGORY, [{
    cat_list: CATEGORY_LIST_ADD,
    trigger_obj: undefined,
    next_trig: undefined
  }]);
};

categoryBoxEvents[EV_ADD_CATEGORY] = function (e, data) {
  add_category(this, e, data);
};

CATEGORY_BOX.on(categoryBoxEvents);

However, don't rely on undefined being undefined. For mysterious reasons, undefined isn't a reserved word in JS, meaning that some (older) runtimes allow it be defined. Instead, use null or just don't set a value at all.

For instance, the event-key event handler above could simply be:

enter_category($(this), EV_ADD_CATEGORY, [{
  cat_list: CATEGORY_LIST_ADD
}]);

If you don't define trigger_obj and next_trig, they're - by definition - undefined.

Where you need to to check for these values, you can simply do

if (!data.trigger_obj) { ... }

because undefined is false'y (and so is false, null, 0, and "").

Or, if you feel you should be (needlessly) exact:

if (typeof data.trigger_obj === 'undefined') { ... }

I'd also skip a lot of the proxy-functions you've got going on. For instance, add_category is called from 2 identical wrapper functions, only so its 1st argument can be set. Don't bother. Try this instead:

function add_category(e, data) {
  var $this = $(this); // just use `this`
  $.ajax({
    url: URL_CATEGORY,
    data: { value: $this.val() },
    success: function (output) {
      data.cat_list.append(output);
      $this.val("");
      if(data.trigger_obj && data.next_trig) {
        data.trigger_obj.trigger(data.next_trig);
      }
    }
  });
}

// ... snip ...

categoryBoxEvents[EV_ADD_CATEGORY] = add_category;

Now, when add_category is called, its this will automatically be the right object. No need for you to specify it. And if you ever need to call add_category in a different context, you can use call() or apply(), or bind it to an object with bind() (not supported on all browsers) or jQuery.proxy().

Lastly, I think you may be a bit trigger-happy (pardon the pun). You're handling events by triggering a lot of other events, which in turn seem to trigger events and so on. While that does decouple your code, it also introduces some overhead. It might make the code simpler to skip some of the trigger-cascades in favor of just calling functions directly. Same goes for the constants, which, while useful in many cases, seem a little excessive. Functions are first-class objects, so you can simply pass them around instead of referring to them.

\$\endgroup\$
  • \$\begingroup\$ I understand that functions can be passed as arguments but where in this code can passing functions can be useful? \$\endgroup\$ – Aseem Bansal Jun 8 '14 at 12:12
  • 1
    \$\begingroup\$ @AseemBansal It's just as an alternative to using trigger everywhere. Triggering events require you to a) trigger an event with a given name on a given obj, and b) have a listener for that same given name on that same given object so function x will be called. Alternatively, you could simply replace all that with a direct call to function x. E.g. bind_events could take a obj of eventname/handler pairs and call them directly, instead of setting up events to call them indirectly \$\endgroup\$ – Flambino Jun 8 '14 at 12:20
  • \$\begingroup\$ OMG I was so excited with AJAX/jQuery that I had actually forgotten that event based programming is not necessary in Javascript. I am still in shock after realizing this by your comment. \$\endgroup\$ – Aseem Bansal Jun 8 '14 at 12:47
  • \$\begingroup\$ As my javascript is currently stuck in triggers how would you suggest going to functions? I am thinking of using bind_events to route to proper functions. As there are many elements in my html setting a switch should be a good idea for selecting events for each element. Any thoughts on this? \$\endgroup\$ – Aseem Bansal Jun 8 '14 at 13:03
  • \$\begingroup\$ @AseemBansal You can perhaps use more or less the same structure, i.e. make an object like { "13": function () {...} }, and, in your bind_events do something like if(typeof handlers[keycode] === 'function') handlers[keycode](); (that's semi-pseudo code, and not far from how trigger etc. works internally, but it's still more direct \$\endgroup\$ – Flambino Jun 8 '14 at 14:40

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.