0

I recently asked this question:

Why I can't update update my JavaScript object properties value?

Which is now solved and works perfectly, here is the updated version on jsFiddle:

http://jsfiddle.net/Farzad/ndb1490v/3/

And here is the code itself:

var app = app || {};

app.layout = (function() {
    var options = {
        header: {
            object: $('#header'),
            visible: false
        },
        content: {
            object: $('#content'),
            visible: false
        },
        sidebarLeft: {
            object: $('.sidebar-left'),
            visible: true
        },
        sidebarRight: {
            object: $('.sidebar-right'),
            visible: false
        },
        footer: {
            object: $('#footer'),
            visible: false
        }
    };
    return {
        // SIDEBAR LEFT
        sidebarLeft: {
            init: function() {
                if(options.sidebarLeft.object.hasClass('active')) {
                    options.sidebarLeft.visible = true;
                }
            }(),
            open: function() {
                if (this.isActive() == false) {
                    options.sidebarLeft.visible = true;
                    options.sidebarLeft.object.addClass('active');
                }
            },
            close: function() {
                if (this.isActive() == true) {
                    options.sidebarLeft.visible = false;
                    options.sidebarLeft.object.removeClass('active');
                }
            },
            isActive: function() {
                return options.sidebarLeft.visible;
            }
        },
        sidebarRight: {
            init: function() {
                if(options.sidebarRight.object.hasClass('active')) {
                    options.sidebarRight.visible = true;
                }
            }(),
            open: function() {
                if (this.isActive() == false) {
                    options.sidebarRight.visible = true;
                    options.sidebarRight.object.addClass('active');
                }
            },
            close: function() {
                if (this.isActive() == true) {
                    options.sidebarRight.visible = false;
                    options.sidebarRight.object.removeClass('active');
                }
            },
            isActive: function() {
                return options.sidebarRight.visible;
            }
        },
        // HEADER
        header: {},
        // CONTENT
        content: {},
        // FOOTER
        footer: {}
    };
})();
////////////////////////////////////////////////////////////////////////////////////////////////////////

// Which panel are active


    if(app.layout.sidebarLeft.isActive() == true){
        app.layout.sidebarLeft.close();
        app.layout.sidebarLeft.open();
    }
    if(app.layout.sidebarRight.isActive() == true){
        app.layout.sidebarRight.close();
        app.layout.sidebarRight.open();
    }


var $notification = $('.notification');



    $notification.html('Left sidebar is active = ' + app.layout.sidebarLeft.isActive() + '<br>' + 'Right sidebar is active = ' + app.layout.sidebarRight.isActive());


    // Left Sidebar Actions
$('#sidebar-left-open').click(function(){
    app.layout.sidebarLeft.open();
});

$('#sidebar-left-close').click(function(){
    app.layout.sidebarLeft.close();
});

// Right Sidebar Actions
$('#sidebar-right-open').click(function(){
    app.layout.sidebarRight.open();
});

$('#sidebar-right-close').click(function(){
    app.layout.sidebarRight.close();
});


    $('button').click(function(){
        $notification.html('Updating...');
        setTimeout(function(){
        $notification.html('Left sidebar is active = ' + app.layout.sidebarLeft.isActive() + '<br>' + 'Right sidebar is active = ' + app.layout.sidebarRight.isActive());
        }, 400);
    });

However I'm not happy with the approach I've taken as I'm repeating myself, (if you look at SidebarLeft and sidebarRight) methods within my app.layout object, you'll see what I mean! I want it to be more flexible NOT repeating my code...

If anybody knows any better approach then could you please help me with it! I don't want the code, please explain where I went wrong and put the basic concept behind it, I want to do it myself and see if I could do it in my own :)

It's just an exercise... Thank in advance

Community
  • 1
  • 1
ZAD
  • 569
  • 2
  • 7
  • 23
  • you could add a function which takes the sidebarLeft or sidebarRight objects and returns an object with init, open etc properties with reference to the passed object as part of closure. – gp. Nov 04 '15 at 12:22

1 Answers1

2

Looking not to repeat yourself? Create a reusable object like so:

var Sidebar = function(selector) {  
    this.activeClass = 'active';
    this.$el = $(selector);
};

Sidebar.prototype.isActive = function() {
    return this.$el.hasClass(this.activeClass);
};

Sidebar.prototype.open = function() {
    this.isActive() || this.$el.addClass(this.activeClass);
};

Sidebar.prototype.close = function() {
    this.isActive() && this.$el.removeClass(this.activeClass);  
};

and then use it like this:

var app = app || {};

app.layout = (function() {
    var options = {
        header: {
            object: $('#header'),
            visible: false
        },
        content: {
            object: $('#content'),
            visible: false
        },
        footer: {
            object: $('#footer'),
            visible: false
        },
        sidebarLeft: new Sidebar('.sidebar-left'),
        sidebarRight: new Sidebar('.sidebar-right'),
    };

    return {
       sidebarLeft,
       sidebarRight,
       header:{},
       content:{},
       footer:{}
    }
}());

//the rest of your code

You can also see that your visible flag is redundant and needs to be set every time you call open or close. You can completely replace it by calling isActive() instead of using the visible flag.

EDIT

Example of what you asked for in your comments - use an IIFE around everything to prevent things from being globally accessible:

var app = app || {};
(function() {

    /* sidebar constructor and prototype methods */
    var Sidebar = function() { ... }

    app.constructors = app.constructors || {};
    app.constructors.Sidebar = Sidebar;

    app.layout = ...

}());
Adam Jenkins
  • 51,445
  • 11
  • 72
  • 100
  • Thanks Adam, really appreciate it :) ___ one question: would be possible to have the sidebar constructor within my app.layout object, the reason being it that I don't want it to be globally accessible! Also is it better practice to put all of my app constructors under a single object like; app.constructors or something like that? Would you recommend that... so when I'm creating a new instance of that constructor I could do; ` sidebarLeft = new app.constructors.Sidebar(".sidebar-left"); ` __ THANK YOU – ZAD Nov 04 '15 at 12:40
  • @FarzadCyrus - sure, see my edit. With regards to using a constructors object, I definitely wouldn't say it's better practice, but there's nothing necessarily wrong with it, I suppose. Honestly, it seems more like over-engineering to do it that way, but that's just an opinion (as will be any answer you get on that front). – Adam Jenkins Nov 04 '15 at 12:59