5
\$\begingroup\$

I am working on a personal project for managing my bookmarks, which is basically a web page to manage my bookmarks by categories. Managing here means everything - adding, viewing, updating, searching by categories, adding new categories.

As can be guessed there is need of AJAX and JavaScript. I have a JavaScript file for managing this interactivity on the webpage. I am a newbie when it comes to JS/jQuery/AJAX so I have doubts about the quality of the code that I have written. I decided to get part of it reviewed. I got an answer due to which I decided to refactor the whole thing. I am placing both files here for review.

The first file is largely based on triggers. The second one has functions instead of using triggers. I want to know whether there is any improvement or not in any aspect. Or is the old one better in any aspect. Any suggestions are ok.

I am not willing to lose loose coupling for anything as it makes it a lot harder for me to reason about the flow and make changes.

If someone wants to see complete code for project then it is here. If anyone wants to see the below code as gists then they are here.

PRE

//Variables for URLs
var URL_CATEGORY_AUTO = "auto/category/";
var URL_BOOKMARK_AUTO = "auto/bookmark/";
var URL_PAGE_OPEN = "open/";
var URL_BOOKMARK_LIST = "bookmark/category/";
var URL_BOOKMARK_BY_NAME = "bookmark/name/";
var URL_CATEGORY = "category/";

//Variables for selector strings
var CLASS_CATEGORY = ".category";
var CLASS_DEL_CATEGORY = ".delete-cat";
var CLASS_BOOKMARK = ".bookmark";
var CLASS_UI_MENU_ITEM = ".ui-menu-item";

//Variables for getting length of classes
var LEN_DEL_CATEGORY = "delete-cat ".length;
var LEN_BOOK_CATEGORY = "col-md-12 bookmark ".length;

//Variables for selectors
var CATEGORY_INPUT = $("#category_inp");
var BOOKMARK_LIST = $("#bookmarks-list");
var CATEGORY_LIST_SEARCH = $("#category_list_search");
var CATEGORY_LIST_ADD = $("#category_list_add");
var BOOKMARK_NAME = $("#bookmark-name");
var CATEGORY_BOX = $("#category-box");
var TOP_WRAPPER = $("#top-wrapper");
var ADD_BOOKMARKS = $("#add-bookmark");

//Variables for event names
var EV_ADD_CATEGORY = "addCategory";
var EV_CLICK = "click";
var EV_UPDATE_BOOKMARKS = "updateBookmarks";
var EV_CLEAR_ALL = "clearAll";


//Auxiliary functions
function valueInSelector(valueOf, selector) {
    var result = false;
    $(selector).each(function () {
        if (valueOf.val() == $.trim($(this).text())) {
            result = true;
        }
    });
    return result;
}

function getClassString(curObj, start) {
    return $(curObj).attr('class').substr(start);
}

function bindEvents(curObj, e) {
    var triggerName = "";
    if (e.keyCode == 13) {
        triggerName = "enterKey";
    } else if (e.altKey && e.keyCode == "N".charCodeAt(0)) {
        triggerName = "clickAltN";
    }
    $(curObj).trigger(triggerName);
}

//Action for body Load
$(function () {
    TOP_WRAPPER.toggle();

    CATEGORY_INPUT.autocomplete({
        source: function (req, res) {
            $.getJSON(URL_CATEGORY_AUTO, { excludes: getAllCategories(), term: CATEGORY_INPUT.val() },
                res);
        }
    });

    CATEGORY_BOX.autocomplete({
        source: function (req, res) {
            $.getJSON(URL_CATEGORY_AUTO, { excludes: getAllCategories(), term: CATEGORY_BOX.val() },
                res);
        }
    });

    BOOKMARK_NAME.autocomplete({
        source: URL_BOOKMARK_AUTO
    });
});

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

function testCategory(categoryInp) {
    if (valueInSelector(categoryInp, CLASS_UI_MENU_ITEM) == false) {
        alert("Not a category");
    } else if (valueInSelector(categoryInp, CLASS_CATEGORY) == true) {
        alert("Category already added");
        categoryInp.val("");
    } else {
        return true;
    }
    return false;
}

function addCategory(curObj, categoryList, triggerData) {
    $.ajax({
        url: URL_CATEGORY,
        data: {'value': $(curObj).val()},
        success: function (output) {
            categoryList.append(output);
            $(curObj).val("");
            if (triggerData.trigger_obj !== undefined) {
                triggerData.trigger_obj.trigger(triggerData.next_trig);
            }
        }
    });
}

function deleteCategory(curObj) {
    var categoryId = getClassString(curObj, LEN_DEL_CATEGORY);
    $(CLASS_CATEGORY + "." + categoryId).remove();
}

//Actions common to both category boxes
$("#category_inp,#category-box").on({
    keyup: function (e) {
        bindEvents(this, e);
    },
    "enterKey": function () {
        if (testCategory($(this))) {
            $(this).trigger(EV_ADD_CATEGORY);
        }
    },
    "clearAll": function () {
        $(this).val("");

    }
});

//Actions for Search's category box
CATEGORY_INPUT.on({
    "addCategory": function () {
        addCategory(this, CATEGORY_LIST_SEARCH, {
            trigger_obj: BOOKMARK_LIST,
            next_trig: EV_UPDATE_BOOKMARKS
        });
    }
});

//Actions for Add Bookmark's category box
CATEGORY_BOX.on({
    "addCategory": function () {
        addCategory(this, CATEGORY_LIST_ADD, {});
    }
});

CATEGORY_LIST_SEARCH.on({
    "clearAll": function () {
        $(this).html("");
        BOOKMARK_LIST.trigger(EV_CLEAR_ALL);
    }
});

CATEGORY_LIST_ADD.on({
    "clearAll": function () {
        $(this).html("");
    }
});

//Actions for delete of Category - Search
CATEGORY_LIST_SEARCH.on(EV_CLICK, CLASS_DEL_CATEGORY, function () {
    deleteCategory(this);
    BOOKMARK_LIST.trigger(EV_UPDATE_BOOKMARKS);
});

//Actions for delete of Category - Add
CATEGORY_LIST_ADD.on(EV_CLICK, CLASS_DEL_CATEGORY, function () {
    deleteCategory(this);
});

//Actions for bookmark List
BOOKMARK_LIST.on({
    "updateBookmarks": function () {
        $.ajax({
            url: URL_BOOKMARK_LIST,
            data: {'ids': getAllCategories()},
            success: function (output) {
                BOOKMARK_LIST.html(output);
            }
        });
    },

    "clearAll": function () {
        $(this).html("");
    }
});

//Actions for Bookmark
BOOKMARK_LIST.on(EV_CLICK, CLASS_BOOKMARK, function () {
    $.ajax({
        url: URL_PAGE_OPEN,
        data: {'id': getClassString(this, LEN_BOOK_CATEGORY)}
    });
});

//Actions for Show/Hide Bookmark addition screen
ADD_BOOKMARKS.on({
    click: function () {
        TOP_WRAPPER.toggle();
        CATEGORY_INPUT.trigger(EV_CLEAR_ALL);
        CATEGORY_LIST_SEARCH.trigger(EV_CLEAR_ALL);
    }
});

//Action for BookMark Name - WIP
BOOKMARK_NAME.on({
    keyup: function (e) {
        bindEvents(this, e);
    },

    "enterKey": function () {
        $.ajax({
            url: URL_BOOKMARK_BY_NAME,
            data: {'name': BOOKMARK_NAME.val()},
            success: function (output) {
                for (var category in output) {
                    CATEGORY_BOX.trigger(EV_ADD_CATEGORY);
                }
            }
        })
    }
});

POST

//Variables for URLs
var URL_CATEGORY_AUTO = "auto/category/";
var URL_BOOKMARK_AUTO = "auto/bookmark/";
var URL_PAGE_OPEN = "open/";
var URL_BOOKMARK_LIST = "bookmark/category/";
var URL_BOOKMARK_BY_NAME = "bookmark/name/";
var URL_CATEGORY = "category/";

//Variables for selector strings
var CLASS_CATEGORY = ".category";
var CLASS_DEL_CATEGORY = ".delete-cat";
var CLASS_BOOKMARK = ".bookmark";
var CLASS_UI_MENU_ITEM = ".ui-menu-item";

//Variables for getting length of classes
var LEN_DEL_CATEGORY = "delete-cat ".length;
var LEN_BOOK_CATEGORY = "col-md-12 bookmark ".length;

//Auxiliary Functions
function valueInSelector(valueOf, selector) {
    var result = false;
    $(selector).each(function () {
        if (valueOf.val() == $.trim($(this).text())) {
            result = true;
        }
    });
    return result;
}

function getClassString(curObj, start) {
    return $(curObj).attr('class').substr(start);
}

function initSelector(uniqueId, customId) {
    var selector = $(uniqueId);
    selector.attr('customId', customId);
    return selector;
}

//Variables for selector Strings
var strCategoryInput = 'CATEGORY_INPUT';
var strCategoryListSearch = 'CATEGORY_LIST_SEARCH';
var strTopWrapper = 'TOP_WRAPPER';
var strBookmarkName = 'BOOKMARK_NAME';
var strCategoryBox = 'CATEGORY_BOX';
var strCategoryListAdd = 'CATEGORY_LIST_ADD';
var strAddBookmarks = 'ADD_BOOKMARKS';
var strBookmarkList = 'BOOKMARK_LIST';

//Variables for selectors
var CATEGORY_INPUT = initSelector("#category_inp", strCategoryInput);
var CATEGORY_LIST_SEARCH = initSelector("#category_list_search", strCategoryListSearch);
var TOP_WRAPPER = initSelector("#top-wrapper", strTopWrapper);
var BOOKMARK_NAME = initSelector("#bookmark-name", strBookmarkName);
var CATEGORY_BOX = initSelector("#category-box", strCategoryBox);
var CATEGORY_LIST_ADD = initSelector("#category_list_add", strCategoryBox);
var ADD_BOOKMARKS = initSelector("#add-bookmark", strAddBookmarks);
var BOOKMARK_LIST = initSelector("#bookmarks-list", strBookmarkList);

//Controller functions - START
function eventClear(curObj) {
    var customId = curObj.attr("customId");
    switch (customId) {
        case strCategoryInput:
        case strCategoryBox:
            curObj.val("");
            break;
        case strCategoryListSearch:
            eventClear(BOOKMARK_LIST);
        case strCategoryListAdd:
        case strBookmarkList:
            curObj.html("");
            break;
        default:
            alert("eventClear for " + customId + " Not Bound");
    }
}

function eventEnter(e, curObj) {
    var customId = curObj.attr("customId");
    switch (customId) {
        case strCategoryInput:
        case strCategoryBox:
            if (testCategory(curObj)) {
                eventAddCategory(curObj);
            }
            break;
        default:
            alert("eventEnter for " + customId + " Not Bound");
    }
}

function eventAddCategory(curObj) {
    var customId = curObj.attr("customId");
    switch (customId) {
        case strCategoryInput:
            addCategory(curObj, CATEGORY_LIST_SEARCH, updateBookmarks);
            break;
        case strCategoryBox:
            addCategory(curObj, CATEGORY_LIST_ADD, null);
            break;
        default:
            alert("eventAddCategory for " + customId + " Not Bound");
    }
}

function eventClick(e, curObj) {
    var customId = curObj.attr("customId");
    switch (customId) {
        case strAddBookmarks:
            bindEvents(e, ADD_BOOKMARKS);
            TOP_WRAPPER.toggle();
            eventClear(CATEGORY_INPUT);
            eventClear(CATEGORY_LIST_SEARCH);
            break;
        default:
            alert("eventClick for " + customId + " Not Bound");
    }
}

function bindEvents(e, curObj) {
    if (e.keyCode == 13) {
        eventEnter(e, curObj);
    } else if (e.altKey && e.keyCode == "N".charCodeAt(0)) {
    }
}

$(function () {
    TOP_WRAPPER.hide();

    CATEGORY_INPUT.autocomplete({
        source: function (req, res) {
            $.getJSON(URL_CATEGORY_AUTO, { excludes: getAllCategories(), term: CATEGORY_INPUT.val() },
                res);
        }
    });

    CATEGORY_BOX.autocomplete({
        source: function (req, res) {
            $.getJSON(URL_CATEGORY_AUTO, { excludes: getAllCategories(), term: CATEGORY_BOX.val() },
                res);
        }
    });

    BOOKMARK_NAME.autocomplete({
        source: URL_BOOKMARK_AUTO
    });
});

//Auxiliary functions for Categories - START
function getAllCategories() {
    var classIds = [];
    $(CLASS_DEL_CATEGORY).each(function () {
        classIds.push(getClassString(this, 11));
    });
    return classIds.join(",");
}

function testCategory(categoryInp) {
    if (valueInSelector(categoryInp, CLASS_UI_MENU_ITEM) == false) {
        alert("Not a category");
    } else if (valueInSelector(categoryInp, CLASS_CATEGORY) == true) {
        alert("Category already added");
        categoryInp.val("");
    } else {
        return true;
    }
    return false;
}

function addCategory(curObj, list_update, func) {
    $.ajax({
        url: URL_CATEGORY,
        data: {'value': $(curObj).val()},
        success: function (output) {
            list_update.append(output);
            $(curObj).val("");
            if (func !== null) {
                func();
            }
        }
    });
}

function deleteCategory(curObj) {
    var categoryId = getClassString(curObj, LEN_DEL_CATEGORY);
    $(CLASS_CATEGORY + "." + categoryId).remove();
}

//Auxiliary functions for Bookmarks
function updateBookmarks() {
    $.ajax({
        url: URL_BOOKMARK_LIST,
        data: {'ids': getAllCategories()},
        success: function (output) {
            BOOKMARK_LIST.html(output);
        }
    });
}

//jQuery for binding events and delegating events START
$("#category_inp,#category-box,#bookmark-name,#add-bookmark").on("keyup", function (e) {
        bindEvents(e, $(this));
    }
);

$("#add-bookmark").on("click", function (e) {
        eventClick(e, $(this));
    }
);

//TODO Find a way to refactor this event delegation properly
//Actions for delete of Category - Search
CATEGORY_LIST_SEARCH.on("click", CLASS_DEL_CATEGORY, function (e) {
    deleteCategory(this);
    updateBookmarks();
});

//Actions for delete of Category - Add
CATEGORY_LIST_ADD.on("click", CLASS_DEL_CATEGORY, function (e) {
    deleteCategory(this);
});

//Actions for Bookmark
BOOKMARK_LIST.on("click", CLASS_BOOKMARK, function (e) {
    $.ajax({
        url: URL_PAGE_OPEN,
        data: {'id': getClassString(this, LEN_BOOK_CATEGORY)}
    });
});
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

The Good

  • Naming conventions seem to be consistent
  • The programming pattern seems to be consistent

Basically, the code just looks consistently written. That being said, it's not really organized.

The Bad

  • All variables are global
  • All variables appear to be "constants"
  • All functions are global
  • All the code put together performs a mishmash of jobs

The Ugly

Nothing is really ugly here. For that you'd need a helluva lot more Clint Eastwood.

Breaking Down Your Application Into Components

To really organize your code, you need to break it down into components that focus on one specific task, for example, adding a category. Looking at your code, I can see the following tasks:

  • Showing all categories
  • Showing bookmarks in a category
  • Adding a category
  • Editing a category
  • Removing a category
  • Adding a bookmark
  • Editing a bookmark
  • Removing a bookmark

You can break your application down into several "controllers":

  • CategoryIndexController: Shows a list of all categories
  • BookmarkIndexController: Shows the bookmarks
  • CategoryController: Adds, edits and removes categories
  • BookmarkController: Adds, edits and removes bookmarks

When you change anything with categories, you need to refresh the category list. Same thing with bookmarks. For this you can use events. Controllers would publish an event, say "category:updated". The CategoryIndexController would subscribe to "category:updated" and refresh the category list. Same idea with BookmarkIndexController.

Now the AJAX functionality is currently all global. I usually go for the Repository Pattern to encapsulate AJAX calls. For that you would have two more classes:

  • CategoryRepository: Provides methods for the basic CRUD operations on categories
  • BookmarkRepository: Provides methods for the basic CRUD operations on bookmarks

This way all AJAX is centralized and abstracted away. None of the rest of the application even needs to know AJAX is at work. You could rewrite your repository layer to use asynchronous calls to the browser's IndexedDB for an offline application and you wouldn't need to refactor any of your other code.

Controllers

Over the years I keep coming back to this basic pattern when creating controllers in JavaScript.

Controllers:

  • Handle all the user interaction for a certain root element in the document on inwards
  • Utilize the repository objects to modify data
  • Publish events on a common event bus when something interesting happens within the controller that the outside world might want to know about
  • Subscribe to events in order to respond to events that other controllers publish

Some pseudo code outlining this pattern is below:

BookmarkIndexController
function BookmarkIndexController(element) {
    this.element = typeof element === "string" ? document.getElementById(element) : element;
    this.document = element.ownerDocument;
    this.$document  $(this.document);
    this.$element = $(element);

    this.handleBookmarkUpdated = this.handleBookmarkUpdated.bind(this);
    this.bookmarks = new BookmarkRepository();
}
BookmarkIndexController.prototype = {

    bookmarks: null,

    element: null,

    $element: null,

    constructor: BookmarkIndexController,

    init: function() {
        this.$document.on("bookmark:updated", this.handleBookmarkUpdated);

        this.bookmarks.findAll()
            .done(function(html) {
                this.$element.html(html);
            }.bind(this);
    },

    handleBookmarkUpdated: function(event) {
        this.bookmarks.findAll()
            .done(function(html) {
                this.$element.html(html);
            }.bind(this);
    }
}
BookmarkRepository
function BookmarkRepository() {
}
BookmarkRepository.prototype = {

    baseUrl: "/bookmarks",

    findAll: function() {
        return $.ajax({
            type: "GET",
            url: this.baseUrl + "/index"
        });
    }
}

And some HTML to kick things off:

<div id="bookmarks-list"></div>
<script type="text/javascript">
    var bookmarkIndex = new BookmarkIndexController("bookmarks-list")
    bookmarkIndex.init();
</script>

Separating things into these layers makes them testable. You could write Jasmine or Mocha tests for your controller and repository layer:

describe("BookmarkIndexController", function() {

    var element, controller, repository, promise;

    beforeEach(function() {
        element = document.createElement("div");

        repository = new BookmarkRepository();

        controller = new BookmarkIndexController(element);
        controller.repository = repository;

        promise = {
            done: function(callback) {
                this.callback = callback;
            }
        };
    });

    it("loads bookmarks", function() {
        spyOn(repository, "findAll").and.returnValue(promise);
        spyOn(promise, "done").and.callThrough();

        controller.init();

        // Mock AJAX request
        promise.callback("Bookmarks go here!");

        expect(promise.done).toHaveBeenCalled();
        expect(element.innerHTML).toBe("Bookmarks go here!");
    });

});

I know this is a lot to throw at a code review, but this does allow for loose coupling, plus all the functionality is componentized and reusable. Since each class has all of its dependencies as public properties (like the repository) it becomes easy to mock up those dependencies for the purpose of unit testing.

\$\endgroup\$
6
  • \$\begingroup\$ I am assuming you are talking OOPS and by tasks you don't mean a function task but an abstraction for an element. Not html element but conceptual element for the webpage. I am newbie to Javascript so let me ask a silly question. The concept of event not same as triggering an event on an element through jQuery like I have done in the first implementation. Right? Didn't get the part about "Handle all the user interaction". Rest... well it went over my head. Sorry. I am not able to understand how you split these up. I have never declared a prototype before in Javascript so I will check that. \$\endgroup\$ Commented Jun 10, 2014 at 17:34
  • \$\begingroup\$ I will read this answer again after understanding Javascript more. Javascript 101 seems like a good palce to go. As I have never actually went through the basics of Javscript before can you suggest any other place that I can check? \$\endgroup\$ Commented Jun 10, 2014 at 17:36
  • \$\begingroup\$ By tasks I mean "things your program does" --- task being something very conceptual. A task a program does can usually be written in plain language, for instance "As a user, I can add a category" or "As a user, I can view all bookmarks in a category". \$\endgroup\$ Commented Jun 10, 2014 at 19:04
  • \$\begingroup\$ By "handle all user interaction" I mean responding to DOM events like "click" and "submit". \$\endgroup\$ Commented Jun 10, 2014 at 19:05
  • 4
    \$\begingroup\$ For a moment, disconnect yourself from "JavaScript" and think more in terms of programming patterns in general. MVC (Model-View-Controller) and the Repository Pattern are all general descriptions of problems with general descriptions of solutions, rather than code you actually copy and paste. Think about the dashboard on a car. Not every dashboard looks the same, but all the same parts are there: Speedometer, gas gauge, etc. The dashboard of a car is a Design Pattern --- a generic description with each auto maker implementing their own vision of a dashboard. \$\endgroup\$ Commented Jun 10, 2014 at 19:14

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.