Navigation bar built from fetched JSON data

I have finally been able to create my little navigation plugin in an object-oriented way that reads the li from a JSON file.

Now, is my code efficient? Especially, the click function within getRegion function?

Is this the correct way to handle the click events in OO?

Here's is working version: PLNKR

```
'use strict';
if (typeof Object.create !== 'function') {

Object.create = function (obj) {
function F () {
}
F.prototype = obj;
return new F ();
};
}

(function ($, window, document, undefined) {

var Navigation = {
init: function (options, elem) {
var self = this;

self.elem = elem;
self.$elem = $ (elem);

self.url = 'data.json';

self.navigation = ( typeof options === 'string' )
? options
: options.navigation;

self.options = $.extend ({}, $.fn.drawNavigation.options, options);

self.cycle ();
},

cycle: function () {
var self = this;
self.fetch ().done (function (results) {
self.buildFrag (results);
self.display ();
self.getRegion ();
});
},

buildFrag: function (results) {
var self = this;
var navigationRegion = self.options.navigation;
self.list = $.map (results[navigationRegion].navigation, function (obj, i) {
return $ (self.options.wrapEachWith).append (obj);
});
},

fetch: function () {
return $.ajax ({
url: this.url
});
},

display: function () {
this.$elem.hide().html (this.list).fadeIn(800);
},

activate: function (item) {
$ (item).siblings ().removeClass ('flag-active');
$ (item).addClass ('flag-active');
},

getRegion: function () {
var self = this;

Now, is my code efficient?

We don't care. There's not much going on here, there are not thousands of elements in the DOM, and there are no tight data processing loops. If the UI feels sluggish, something must have been gone horribly wrong.

However, there is one inefficiency in your code that matters: The JSON file is fetched on really every single click. I'm pretty sure this is not desired.

It would be OK if you expect it to change really often (that is,

• Using self in your init function is unnecessary. Just use this, as you do in fetch or display.

• Why assign .elem? You never need it.

• The url should definitely not be a constant, but go into the options. 'data.json' may be the default value.

• navigation is not the most descriptive property name. From the usage, I'd expect an adjective like current or selected in the name.

• You use options.navigation to create self.navigation before assigning it a default value.

• cycle is not an optimal name either, it sounds as if it cycles through the options periodically. Name it after what the method actually does - or just inline it into the initialisation code.

• I'm not entirely sure what getRegion does and whether it needs to go into the fetch callback. If you move it outside, you might avoid the code duplication with the fetch in the click handler below.

• You shouldn't use options.navigation for storing the state, but the this.navigation property (which you created in init but never use it) - both in buildFrag and the click handler.

• Why does buildFrag assign to that list property? Instead, return your result from the method! display should then take this as a parameter (or you inline it right away).

• getRegion is misnamed as well. The only code that is "getting the region" is $ (this).attr ('id').split ('_')[0]. What your whole method does is something like setupRegionSelectHandlers or so.

• What is $ ('#flags').find ('div')? This shouldn't be static, make the selector an option. Also, it would get clearer if those selection-sensitive divs were stored in a property similar to .$elem - I assume they're not descendants of each other?

• As said above, the clicks shouldn't fetch the file again. Instead, store the promise for the result on your instance, and reuse that for every display.

I'd do something like this:

(function ($, window, document, undefined) {

    function Navigation(options, elem) {
        if (typeof options === 'string')
            options = {navigation: options};
        this.options = $.extend ({}, $.fn.drawNavigation.options, options);

        this.$elem = $(elem);
        this.$selects = $(this.options.select)

        this.navigatedRegion = this.options.navigation;

        this.fetched = $.ajax({
            url: this.options.url
        });

        this.display();
        this.setupSelectionEvents();
    }

    Navigation.prototype.display = function () {
        var self = this;
        return this.fetched.then(function(results) {
            self.$elem.hide().html(self.buildFrag (results)).fadeIn(800);
        });
    };

    Navigation.prototype.buildFrag = function(results) {
        var self = this;
        return $.map(results[this.navigatedRegion].navigation, function(obj, i) {
            return $(self.options.wrapEachWith).append(obj);
        });
    };

    function activate(item) {
        $(item).siblings().removeClass('flag-active');
        $(item).addClass('flag-active');
    }

    function getRegion(item) {
        return $(item).attr('id').split('_')[0];
    }

    Navigation.prototype.setupSelectionEvents = function() {
        var self = this;
        this.$selects.on('click', function () {
            activate(this);
            self.navigatedRegion = getRegion(this);
            self.display();
        });
    };

    $.fn.drawNavigation = function(options) {
        return this.each(function() {
            new Navigation(options, this);
        });
    };

    //default
    $.fn.drawNavigation.options = {
        navigation: 'uk',
        wrapEachWith: '',
        select: '#flags div',
        url: 'data.json'
    };

}(jQuery, window, document));

StackExchange Code Review Q#69016, answer score: 6