Learning OOP javascript - critique my class that creates a 'favourites' list

I'm trying to improve my javascript coding style and want to start writing my code more object-oriented. The code below is getting a JSON-stringified list of locations from a cookie and appending a li to a list for each location. I would appreciate some helpful critique and any general pointers. I have some questions too:

-
Am I using self correctly? It feels like I'm over using it i.e. on function calls but when its not there I cannot access those functions. Some plugins I've used are written in this way and some are not, why is that?

-
If I wanted to use this as a static class, how can I code it differently instead of instantiating it?

I have experience with vb/c# if that helps when explaining things.

```
var favourites = function(list, settings) {
var self = this;

self.options = {
maxitems: 5,
cookiename: "fav-locations"
};
self._$list = $(list) || $();
self._locations = [];

//extend defaults with any settings
$.extend(self.options, settings);

//load the locations
var init = function (aryLocations) {
var index = 0;
if (aryLocations) {
for (index; index -1) {
self._locations.splice(location_index, 1);
$.cookie(self.options.cookiename, JSON.stringify(self._locations), { path: '/' });
self.setCount();
}
else {
console.log('location not found.');
}
};

self.populateLocations = function () {
self.clearList();

$.each(self._locations, function (index, item) {

//get this locations name and id - format here is: name|id
var location = {
name: item.split('|')[0] || '',
id: item.split('|')[1] || 0
};

//create a new li element
var $thisItem = $('').appendTo(self._$list[0]).addClass('wrap');

$thisItem.data("loc", item);

$thisItem.html('× ' + location.name + '');

Some things to consider (after a first, superficial look at your code):

• Stick to the conventions: A constructor starts with an UpperCase, and is camelCased from their on. Favourites would be a better var name, because it signals a constructor, and requires the new keyword.

• self, though not a reserved keyword is used by some JS engines for particular purposes. using self can lead to unexpected behaviour. Perhaps consider using var that = this; instead.

• Use the prototype for methods, to avoid creating too many function objects.

• self isn't required all the time, but it's safer. You're not using it badly, you're using it, but don't seem to know what it's for.

• Binding event handlers (self.bindRemoveButtons) is not the job of an object, or any of its methods. It's not because you're using the new keyword, and methods, that you're writing good, OO code. Besides, JS is prototype-based, using traditional OO techniques in JS is like a barber, using a scythe to cut your hair...

• The init function needn't be declared for each instance, use a closure.

• You have no guarantee that $ will be the jQuery object, use a closure.

Now, some examples to help you on your way:

var Favourites = (function($)//pass jQ object to closure
{
    'use strict';
    var init = function()
    {
        //the init function here.
    };
    function Favourites(list, settings)
    {//properties here...
        this._$list = $(list) || $();
        init();//call closure function init declared above ^^
    }
    Favourites.prototype.setCount = function()
    {
        //methods belong to the prototype
    };
    return Favourites;//return reference to constructor
}(jQuery));

This is what a more complex constructor definition in JS looks like.

Update:

Your updated code looks good. Honestly. It's not perfect, but then, in programming, perfection is more of a subjective thing. Personally, I rarely, if not never write constructors. I just use object literals and the module pattern.

You've looked into closures. Great, I get the impression you get what they're for, but you don't yet see all the use-cases for them. For example the bindLocationLinkHandler method, and for that matter, all the DOM-related methods, contain $(document). What this code does, essentially is call the jQuery init funtion, and constructs a jQuery wrapper object around the document object. Why would you do this over and over? why not wrap all those prototype methods in a simple closure like this:

(function($doc)
{
    Favourites.prototype.bindLocationLinkHandler = function()
    {
        $doc.on('click', ...);
    };
}($(document));//pass jQ wrapped document here

This'll save you a few jQ init calls. Your methods are also binding the same handler. Functions are first-class object in JS, construct them once, and use them everywhere. Replace, for example:

$(document).on("click", ".fav-link", function (e)
{
    var elem = $(this).parent();
    self.forwardToSection(elem.data("location"), elem.data("location-id"), e);
});

with something like:

(function($doc)
{
    var self, handler = function(e)
    {
        var elem = $(this).parent();
        self.forwardToSection(elem.data("location"), elem.data("location-id"), e)
    };
    Favourites.prototype.bindLocationLinkHandler = function()
    {
        self = this;//DANGEROUS... won't always work
        $doc.on('click', '.fav-link', handler);
    };
}($(document)));

Of course, you may have spotted that, owing to there only being a single self closure var for all of the instances, the self reference won't be as reliable. It depends on whatever method you're calling and weather or not it matters if you're calling the method on the same object or not. Either way, this was just to show that you can use closures for anything, simple, primitive variables, as well as functions. In fact, you're doing just that in jQ all the time!:

$('li').each(function()
{//<-- jQ's each method accepts an function as argument
});

To close this rant (few, I'm all over the place), I do have 1 more thing I'd like to point out. I left it out of my initial answer because I don't want to spark a pointless, yet lively debate, but still... In for a penny, in for a pound:

If I'm honest, I'm not too keen on jQuery. There's nothing wrong with jQ, but it's just something that is used too much. If all you're doing is delegate a single event, and perform one or two ajax calls, you don't need jQ, in such a scenario, where you're only using 5% of a monolithic toolkit, you're better off writing a few more lines of code. The performance benefits are, often, noticeable.

In this case, one line in particular got up my nose. You're dabbling with OO JS. The point of OO is to contain certain sets of data and logic in manageable, clearly defined entities (objects). Your object Favourites depends on a toolkit that is anything but modular. An object that relies on an entire lib, j

StackExchange Code Review Q#37146, answer score: 4