2
\$\begingroup\$

I'm trying to create a very simple jQuery plugin in the object-oriented way. Now, I'm not sure whether the code I produced is correct and efficient OO programming. The plugin's aim is to change the navigation ul li based on the clicked country.

jsFiddle

// Widget container
(function($) {
// Widget container plugin
    $.fn.myWidget = function () {
        this.each (function () {
            // Vars
            var item = $ (this);

            // Set events
            item.click (function (e) {
                if (e) e.preventDefault ();
                combine_all(item);
            });
        });
    };

    //uk navigation
    var uk_navigation = '<ul>'
        +'<li id="menu-1">UK stuff</li>'
        +'<li id="menu-2">UK phones</li>'
        +'</ul>';

    //global navigation
    var global_navigation = '<ul>'
        +'<li id="menu-1">EU stuff</li>'
        +'<li id="menu-2">EU phones</li>'
        +'</ul>';

    //italy navigation
    var italy_navigation = '<ul>'
        +'<li id="menu-1">Italy stuff</li>'
        +'<li id="menu-2">Italy phones</li>'
        +'</ul>';

    //germany
    var germany_navigation = '<ul>'
        +'<li id="menu-1">Germany stuff</li>'
        +'<li id="menu-2">Germany phones</li>'
        +'</ul>';


    this.get_region = function(item){
        my_region = $ (item).attr ('id');
        return my_region;
    };

    this.make_active = function(item){
        item.each(function(){
            item.siblings().removeClass('flag-active');
        });
        item.addClass('flag-active');
    }

    this.update_navigation = function(my_region){
        var old_navigation = $('#navigation').find('ul');
        var new_navigation;
        if(my_region==='europe_flag'){
            new_navigation = global_navigation;
        }else if(my_region==='uk_flag'){
            new_navigation = uk_navigation;
        }else if(my_region==='germany_flag'){
            new_navigation = germany_navigation;
        }else{
            new_navigation = italy_navigation;
        }

        $(old_navigation).hide().html(new_navigation).fadeIn(800);

    };

    this.combine_all = function(item){
        this.get_region (item);
        this.update_navigation(my_region);
        this.make_active(item);
    };


})(jQuery);

// Main
$(function() {
    $('#flags').find('div').myWidget();
});
\$\endgroup\$
11
  • \$\begingroup\$ Your code could be better. maybe you wanna look at this: css-tricks.com/snippets/jquery/jquery-plugin-template \$\endgroup\$
    – user733421
    Nov 4, 2014 at 14:22
  • \$\begingroup\$ Thanks guys, but I just need to know whether there is something wrong with my code as I'm a newbie in OO programming especially in jQuery area. How my code could correctly be transformed into OO or perhaps using prototypes :( Your help will be much appreciated. \$\endgroup\$
    – angular_learner
    Nov 4, 2014 at 14:39
  • \$\begingroup\$ Whether I'm using the functions correctly especially this.combine_all = function(item) where I call the other functions. Is this a good practice? \$\endgroup\$
    – angular_learner
    Nov 4, 2014 at 14:45
  • \$\begingroup\$ This is in no way an answer, but here is an improved version I am working for you: jsfiddle.net/g6mt2y61/3 but there is still 1 minor bug. (ignore the alert saying "true") \$\endgroup\$ Nov 4, 2014 at 17:45
  • \$\begingroup\$ Here it is a decently updated version: jsfiddle.net/g6mt2y61/5 which has some new utility functions and is (hopefully) well commented. Below, you will have an example of the usage of some utility functions I made. \$\endgroup\$ Nov 4, 2014 at 18:21

2 Answers 2

1
\$\begingroup\$

From a once over:

  • Using each seems overkill, this works:

    $.fn.myWidget = function () {
        this.click( function (e) {
            if (e) {
                e.preventDefault ();
            }
            combine_all(this);
        });
    };
    

    This code assigns the same function to all elements in this, the function has this as the clicked element, so that is why you provide to combine_all

  • JavaScript uses lowerCamelCase so combine_all -> combineAll, also combineAll is not enlightening as function names go
  • Your code is repetitive to determine the links, for starters you could do something like this:

    var country_links = {
      uk     : [ 'UK stuff', 'UK phones' ]
      europe : [ 'EU stuff', 'EU phones' ]
      italy  : [ 'Italy stuff', 'Italy phones' ]
      germany: [ 'Germany stuff', 'Germany phones' ]
    };
    
     this.updateNavigation = function(my_region){
          var oldNavigation = $('#navigation').find('ul'),
              menuItems = country_links[ my_region.split('_')[0] ];
              newNavigation = '<ul>';
           for( var i = 0 ; i < menuItems.length ; i ++ ){
               newNavigation += '<li id="menu-' + (i+1) + '">' + menuItems[i] + '</li>';
           }
           newNavigation += '</ul>'
    
          $(old_navigation).hide().html(new_navigation).fadeIn(800);
     };
    

    This is not the only approach, but you split data from style, country_links has all the data. updateNavigation knows how to build a list from that. I did not test this code but you should catch my drift.

  • To finish on the first point, I would just call all three functions on top instead of combineAll
\$\endgroup\$
2
  • \$\begingroup\$ Many thanks for your solution. Now I need to make it full OO. \$\endgroup\$ Nov 4, 2014 at 19:10
  • \$\begingroup\$ Ok, I think I did it in OOP. Here's my post Do I handle the click event properly? \$\endgroup\$ Nov 5, 2014 at 20:00
0
\$\begingroup\$

Now, I'm not sure whether the code I produced is correct

No. You have coded it so that get_region, my_region, make_active, update_navigation and combine_all are global variables (or properties). If you would have used strict mode, this had thrown exceptions. You would avoid this by not using the this keyword in anything but constructor functions and instance method, and use var statements everywhere.

and efficient OO programming.

I don't see any reason to use OO here. There's no need to have instances of anything around.

Additionally, @konijin has very good points, which I won't reiterate here. You might use more structuring functions than he did if you like that, though.

// Widget container
(function($) {
    // Widget container plugin
    $.fn.myWidget = function() {
        return this.click(function(e) {
//      ^^^^^^ Always return your jQuery collection from methods, for chaining
            e.preventDefault ();
//          ^ no need to check for existence of e - jQuery ensures that one is passed
            updateNavigation($(this));
        });
    };

    var countryLinks = {
        uk:      ['UK stuff', 'UK phones'],
        europe:  ['EU stuff', 'EU phones'],
        italy:   ['Italy stuff', 'Italy phones'],
        germany: ['Germany stuff', 'Germany phones']
    };

    function getNavigationFromFlag(flag) {
        var items = country_links[ flag.split('_')[0] ];
        var newNavigation = '<ul>';
        for (var i = 0; i < menuItems.length; i ++) {
            newNavigation += '<li id="menu-' + (i+1) + '">' + menuItems[i] + '</li>';
        }
        newNavigation += '</ul>';
        return newNavigation;
    }

    function getRegion(item) {
        return $(item).attr('id');
    }

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

    function updateNavigation(item) {
        var navigation = $('#navigation').find('ul');
        var myRegion = getRegion(item);
        var newNavigation = getNavigationFromFlag(myRegion);
        navigation.hide().html(new_navigation).fadeIn(800);
        activate(item);
    }

})(jQuery);

// Main
$(function() {
    $('#flags').find('div').myWidget();
});
\$\endgroup\$
6
  • \$\begingroup\$ Many thanks for the above. It looks much more cleaner than my version. Although, you said no reason for OO. My aim is to learn OO in this particular plugin. How your solution would look like in OO? I very much appreciate your help in advance!! :) \$\endgroup\$ Nov 4, 2014 at 18:45
  • \$\begingroup\$ But there is no application of OO in this situation! \$\endgroup\$
    – Bergi
    Nov 4, 2014 at 18:48
  • \$\begingroup\$ I see. All I wanted to achieve was to be able to write my code even if it's simple in an OO way. Such as these: Tutorial or Tutorial2 where you can use event handlers, init, prototype, etc.. Is it possible? \$\endgroup\$ Nov 4, 2014 at 19:02
  • \$\begingroup\$ Maybe make it so that countryLinks are passed to your plugin function. Move the functions in an OOP class, and instantiate that with the element and the arguments. \$\endgroup\$
    – Bergi
    Nov 4, 2014 at 19:04
  • \$\begingroup\$ See? And this is where I'm lost :( "Move the functions in an OOP class, and instantiate that with the element and the arguments" \$\endgroup\$ Nov 4, 2014 at 19:07

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.