HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Object Oriented Modular Closure

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
orientedobjectclosuremodular

Problem

The following is some code I just wrote. I keep wondering as always what I can do to make this more efficient. Please help me make this code object oriented and if anyone could help me write automated or even non automated tests for my widget...

Also is this considered a closure? I don't think it is but I would love to know how to produce better quality/reusable code. Oh- bonus points for any corrections to my techniques used as well. I'm really trying to step my JS game up and I know it needs work...

var ExplodedContentWidget = $('[data-widget~="exploded-content"]');
// search for existance of the exploded widget in DOM
if (ExplodedContentWidget[0]){
    var ExplodedDefaultEl = $('[data-function~="default-state"]');
    var ExplodedEl = $('[data-function~="exploded-state-1"]');
    var ExplodedEl2 = $('[data-function~="exploded-state-2"]');
    var CloseButton = $('.button-close');
    ExplodedEl.hide(); // default exploded content is hidden
    ExplodedEl2.hide();
    CloseButton.hide();
    ExplodedContentWidget.on('click',function(){
            ExplodedEl.slideToggle('slow');
    });
    var FullScreenViewButton = $('button');
        FullScreenViewButton.on('click',function(){
            ExplodedDefaultEl.hide();
            ExplodedContentWidget.addClass('exploded-fullscreen-state');
            ExplodedEl2.slideToggle('slow');
            CloseButton.fadeIn();
                ExplodedEl2.on('click',function(){
                    ExplodedContentWidget.removeClass('exploded-fullscreen-state');
                    ExplodedDefaultEl.show();
                    ExplodedEl.toggle();
                    ExplodedEl2.toggle();
                    CloseButton.hide();
        });
     });
    }


PS: Here is the fiddle

Solution

From a once over:

  • Do not use data-function~="exploded-state-1", instead assign id's and search by id, this is the common ( and faster, better ) approach



  • One single var statement with comma separated variables is considered better



  • I would go for if (ExplodedContentWidget.length){ instead of if (ExplodedContentWidget[0]){



  • Variable names should follow lowerCamelCase so ExplodedEl -> explodedEl, the exception to that is for jQuery results so you could also use $exploded1



-
This :

ExplodedEl.hide(); // default exploded content is hidden
ExplodedEl2.hide();
CloseButton.hide();


I would have addressed with a css class that hides these elements

  • You indent the code after var FullScreenViewButton = $('button');, dont do that. Consider using the jsbeautifier website



  • $('button');



-
Your code passes JsHint.com nicely, well done

-
Also is this considered a closure? Your inline functions for .on('click' are closures.

Code Snippets

ExplodedEl.hide(); // default exploded content is hidden
ExplodedEl2.hide();
CloseButton.hide();

Context

StackExchange Code Review Q#46087, answer score: 5

Revisions (0)

No revisions yet.