Adding, deleting elements as per autocomplete results - updated

Question

• 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?

Answer 1

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.