4
\$\begingroup\$

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

\$\endgroup\$
5
\$\begingroup\$

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'); <- This is code waiting to go wrong, what if you have more than 1 button. Give the button an id, and select on that id
  • Your code passes JsHint.com nicely, well done

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

\$\endgroup\$
  • \$\begingroup\$ Thanks, I appreciate the help! The button was generic on purpose good point though! That would be a production nightmare haha? Any luck on how I could make this function pass object oriented? \$\endgroup\$ – NicholasAbrams Apr 2 '14 at 19:24
  • \$\begingroup\$ Making it OO is a different ball of wax, I am afraid we don't re-write code, we just review it and sometimes propose an alternative way. \$\endgroup\$ – konijn Apr 2 '14 at 19:37
  • \$\begingroup\$ is there another stackexchange outlet I should be asking this in? Im really just trying to gain the basic concept of this OOJS stuff and would love it if I could see how someone with advanced jquery/js skills would approach the module I have included \$\endgroup\$ – NicholasAbrams Apr 2 '14 at 19:58

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.