3
\$\begingroup\$

I had asked earlier for review of parts of my JS file earlier and review of the complete JS file of my project. Now I have made changes as per the suggestions. Here are the files. I would like to get a review of the structure and organization of the code. I have broken down the earlier single file into multiple files now.

It seems to be clearer but I have the following concerns

  • Is the eventBus useful? Am I using it correctly? I thought that not too many events would fit in the structure here. Am I correct on this?
  • Am I using the repository pattern correctly? Can the use of autocomplete improved here? I mean the way it is being called. Can it be encapsulated completely?
  • Is the problem broken down correctly or are there places where I can still extract further?

auxiliary.js

;
function Common(eventBus, scope, sel) {
    this.eventBus = eventBus;
    this.scope = scope;
    this.sel = sel;
}

function objValueInClass(value, curObj, classString, classIsChild) {
    var result = false;
    var iter = classIsChild ? curObj.sel.find(classString) : $(classString);

    iter.each(function () {
        if (value == $.trim($(this).text())) {
            result = true;
        }
    });
    return result;
}

function getIdsOfChildren(sel, classString) {
    var ids = [];
    sel.find(classString).each(function () {
        ids.push(getClassNames(this, classString.length));
    });
    return ids.join(',');
}

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

category.js

function CategoryInput(eventBus, scope, sel, categoryList) {
    Common.call(this, eventBus, scope, sel);
    this.categoryList = categoryList;

    var _this = this;
    this.sel.on("keyup", function (e) {
        if (e.keyCode == 13) {
            _this.enterKey(e);
        }
    });

    this.sel.autocomplete({
        source: function (req, res) {
            CategoryRepo.AutoByLike(_this.categoryList.getAllCategories(), _this.getVal(), res);
        }
    });
}

CategoryInput.prototype = {
    constructor: CategoryInput,

    enterKey: function (e) {
        var _this = this;
        if (!this.categoryList.testCategory(this)) {
            return;
        }
        CategoryRepo.DetailByName(this.getVal()).success(function (output) {
            _this.categoryList.addCategory(output);
            _this.clear();
        });
    },

    getVal: function () {
        return this.sel.val();
    },

    clear: function () {
        this.sel.val("");
    }
};


function CategoryList(eventBus, scope, sel) {
    Common.call(this, eventBus, scope, sel);
    this.deleteString = ".delete-cat";
}

CategoryList.prototype = {
    constructor: CategoryList,

    addCategory: function (category) {
        var _this = this;
        this.sel.append(category);
        $(this.deleteString).click(function () {
            //TODO Refactor logic for deletion of category
            $(this).closest(".category").remove();
            _this.eventBus.publish(BookmarkerEvent.CategoryDeletedOnPage, _this.scope);
        });
        this.eventBus.publish(BookmarkerEvent.CategoryAddedOnPage, this.scope);
    },

    clear: function () {
        this.sel.html("");
    },

    getAllCategories: function () {
        return getIdsOfChildren(this.sel, this.deleteString);
    },

    testCategory: function (valueSel) {
        var value = valueSel.getVal();
        if (objValueInClass(value, this, ".category", true)) {
            alert("Category already added");
            valueSel.clear();
        } else if (!objValueInClass(value, this, ".ui-menu-item", false)) {
            alert("Not a category");
        } else {
            return true;
        }
        return false;
    }
};

repository.js

function CategoryRepo() {
}

CategoryRepo.AutoByLike = function (exclude, term, res) {
    return $.getJSON(
        "auto/category/",
        {excludes: exclude, term: term},
        res
    );
};

CategoryRepo.DetailByName = function (categoryName) {
    return $.ajax({
        url: "category/",
        data: {'value': categoryName}
    })
};


function BookmarkRepo() {
}

BookmarkRepo.ListByCategory = function (categoryIds) {
    return $.ajax({
        url: "bookmark/category/",
        data: {'ids': categoryIds}
    });
};

BookmarkRepo.OpenPage = function (sel) {
    return $.ajax({
        url: "open/",
        data: {'id': getClassNames(sel, "col-md-12 bookmark ".length)}
    });
};

bookmark.js

function BookmarkList(eventBus, scope, sel, categoryList) {
    Common.call(this, eventBus, scope, sel);
    this.categoryList = categoryList;
}

BookmarkList.prototype = {
    constructor: BookmarkList,

    notify: function (eventName, scope) {
        switch (eventName) {
            case BookmarkerEvent.CategoryAddedOnPage:
            case BookmarkerEvent.CategoryDeletedOnPage:
                this.isCurrentList(scope) && this.updateBookmarks();
                break;
        }
    },

    clear: function () {
        this.sel.html("");
    },

    isCurrentList: function (scope) {
        return this.categoryList.scope == scope;
    },

    updateBookmarks: function () {
        var sel = this.sel;
        BookmarkRepo.ListByCategory(this.categoryList.getAllCategories())
            .success(function (output) {
                sel.html(output);
                $('.bookmark').click(function () {
                    BookmarkRepo.OpenPage(this);
                });
            });
    }
};

section.js

function TopSection(eventBus, scope, sel) {
    this.eventBus = eventBus;
    this.scope = scope;
    this.sel = sel;
}

TopSection.prototype = {
    constructor: TopSection,

    notify: function (eventName, scope) {
        switch (eventName) {
            case BookmarkerEvent.ToggleTopBar:
                this.toggle();
                break;
        }
    },

    toggle: function () {
        this.sel.toggle();
    }
};

bookmarker.js

var eventBus;
var topSection;
var categoryInpSearch;
var categoryInpAdd;
var bookmarkList;

function BookmarkerEvent() {
}
BookmarkerEvent.ToggleTopBar = "topBar:toggle";
BookmarkerEvent.CategoryAddedOnPage = "category:added";
BookmarkerEvent.CategoryDeletedOnPage = "category:deleted";

$(function () {
    eventBus = new BookmarkerEventBus();
    topSection = new TopSection(eventBus, 'topBar', $("#top-wrapper"));

    var categoryListSearch = new CategoryList(eventBus, 'sideBar', $("#category_list_search"));
    categoryInpSearch = new CategoryInput(eventBus, 'sideBar', $("#category_inp"), categoryListSearch);
    var categoryListAdd = new CategoryList(eventBus, 'topBar', $("#category_list_add"));
    categoryInpAdd = new CategoryInput(eventBus, 'topBar', $("#category-box"), categoryListAdd);

    bookmarkList = new BookmarkList(eventBus, 'mainBody', $("#bookmarks-list"), categoryListSearch);

    var subObj = {};
    subObj[BookmarkerEvent.ToggleTopBar] = [topSection];
    subObj[BookmarkerEvent.CategoryAddedOnPage] = [bookmarkList];
    subObj[BookmarkerEvent.CategoryDeletedOnPage] = [bookmarkList];
    eventBus.subscribe(subObj);

    eventBus.publish(BookmarkerEvent.ToggleTopBar, '');
});

$("#add-bookmark").click(function () {
    eventBus.publish(BookmarkerEvent.ToggleTopBar, '');
});
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I guess people are not too keen on this. I will add my own two cents which came to my mind and which I have added to the code. If anyone is interested they can see the code at my github.

In the code a variable scope has been used only for the purpose of differentiating between the different category lists. This variable makes the eventBus a little less useful. I mean why can the object not raise the proper event instead of having the eventBus to do something other than mediate between the objects? It can be solved by adding more events and raising the proper events through an auxiliary function. Encapsulate the type of the category list and based on that raise proper events.

Second thing is regarding the way the events are declared and subscribers are added. It would be better if a common pattern is used when declaring and subscribing. The use of function would be better.

\$\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.