6
\$\begingroup\$

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;
            $ ('#flags').find ('div').on ('click', function () {
                //activate flag
                self.activate (this);
                self.options.navigation = $ (this).attr ('id').split ('_')[0];
                self.fetch ().done (function (results) {
                    self.buildFrag (results);
                    self.display ();
                });
            });

        }
    };

    $.fn.drawNavigation = function (options) {
        return this.each (function () {
            var navigationBar = Object.create (Navigation);
            navigationBar.init (options, this);
        });
    };

    //default
    $.fn.drawNavigation.options = {
        navigation: 'uk',
        wrapEachWith: '<li></li>'
    }

}) (jQuery, window, document);

$ (function () {
    $('#navigation').find('ul').drawNavigation({});
});
\$\endgroup\$
6
\$\begingroup\$

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, < 15min); or if the response would be dynamically created and you did transmit the currently selected option as a parameter.

Especially, the click function within getRegion function? Is this the correct way to handle the click events in OO?

Yes, that's fine.


However, a few oddities I found in your code:

  • You use Object.create and an init function. That's OK if you want to do it that way, and good to understand prototypical inheritance. Yet, I think using a constructor function and new is simpler, as it requires only a single call for instantiation.
  • 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: '<li></li>',
        select: '#flags div',
        url: 'data.json'
    };

}(jQuery, window, document));
\$\endgroup\$
  • \$\begingroup\$ Brilliant. Many thanks for your feedback.You some very good ideas here. However, when I try to run your code solution it does not work. The problem starts in this line 'this.$selects = $(this.options.select)' Can you help me to run your code please? \$\endgroup\$ – angular_learner Nov 6 '14 at 10:22
  • \$\begingroup\$ Doesn't the selector work? How does your HTML look like? \$\endgroup\$ – Bergi Nov 6 '14 at 10:28
  • \$\begingroup\$ ok, I fixed it. The problem was self.options = $.extend ({}, $.fn.drawNavigation.options, options); I used this instead self \$\endgroup\$ – angular_learner Nov 6 '14 at 10:29
  • 1
    \$\begingroup\$ Yes, you can. But you might want to rename it to just setupEvents then… \$\endgroup\$ – Bergi Nov 6 '14 at 10:46
  • 1
    \$\begingroup\$ You mean it's called before the DOM <li>s are created? But there's a solution for that as well. \$\endgroup\$ – Bergi Nov 6 '14 at 22:11

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.