Understanding module design patterns in javascript

Question

I am trying to understand module patterns in Javascript so that i can separate my code into different modules and use them where required.

var messageHandler = (function(){
    var el;
    var display = function(a){
        if(a=='error'){
            el = $('.error');
            el.css('display','block');
        }
        else if (a==='success'){
            el = $('.success');
            el.css('display','block');
        }
        else if (a=='warning'){
            el = $('.warning');
            el.css('display','block');
        }
        else if (a=='danger'){
            el = $('.danger');
            el.css('display','block');
        }
        registerClick(el.find('.close'));
        return this;
    }
    function registerClick(p_el){
        p_el.bind('click',function(){
            hide();
        });
    }
    var hide = function(){
        el.css('display','none');
    }
    return {
        display: display,
        hide: hide
    }
})();
window.messageHandler = messageHandler;

messageHandler.display('warning');

So, I have four different classes in css for different types of messages.The close class is for a small cross button on the top right to close the message. This works fine till i call the function only once.When i do this

messageHandler.display('warning');
 messageHandler.display('success');

Now both the messages close button have been bind to the success close button because el gets overwritten.

How to achieve it keeping the code reusable and concise.

For starters, how about refactoring to el = $('.' + a); and simply do an el.show() at the end to show it? — Johan
– Johan, Commented Jan 24, 2015 at 19:47
Just let your hide function take a parameter (the element), and don't make your el module-wide available. Just like you have done with p_el for registerClick. — Bergi
– Bergi, Commented Jan 24, 2015 at 19:47

JLRishe · Accepted Answer · 2015-01-24 20:26:06Z

2

The problem here is that you have a closure variable el that you are overwriting every time display() is called. The hide() function uses whatever is the current value of el at the time it is called, so overwriting el is a problem.

If you want to have "static" functionality like this display() method, you need to avoid shared state.

As @Bergi points out in the comments, you can eliminate the shared el and modify hide() to take an element as input:

var messageHandler = (function(){
    var el;     // delete this
    var display = function(a){
        var el; // add this

function registerClick(el){
    el.bind('click', function(){
        hide(p_el);
    });
}
function hide(el){
    el.css('display','none');
}

You could also modify hide to make use of the current event properties, and then just have:

function registerClick(el){
    el.bind('click', hide);
}

function hide(event){
    $(event.target).css('display','none');
}

Cleaned up version including the auto-hide discussed in the comments:

var messageHandler = (function(){
    var display = function(a){
        var el = $('.' + a);
        el.css('display', 'block');

        var hideAction = function () { el.css('display', 'block'); };
        var token = setTimeout(hideAction, 5000);

        el.find('.close').bind('click', function () {
            hideAction();
            clearTimeout(token);
        });

        return this;
    }

    return {
        display: display
    }
})();

edited Jan 24, 2015 at 20:26

answered Jan 24, 2015 at 19:53

JLRishe

102k19 gold badges138 silver badges171 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

naman kalkhuria Over a year ago

thanx it works....but i have also made hide method public, so that can use set timeout to hide the message after sometime.I cant pass the el now.

JLRishe Over a year ago

@namankalkhuria Great. I hadn't noticed that you were making the hide method public, but the function hide(event) syntax should still be fine if you are doing that.

JLRishe Over a year ago

@namankalkhuria Ok, good point. I missed that. But in that case, how is hide() supposed to know which element you want to hide? Is it just supposed to hide all the open message boxes?

naman kalkhuria Over a year ago

thats why i had used el as global... so that hide knows which element to hide (works only for one box),,but since we are talking about multiple message box in the page ... i dont know how to achieve it...I ideally i would like both the messages go away after say 5 seconds,good point about hideall .. i can add a hideall public and call it when i have more than one boxes... thnx for the input

JLRishe Over a year ago

@namankalkhuria In that case, how about having the module auto-hide it after 5 seconds? Please see my edit.

Collectives™ on Stack Overflow

Understanding module design patterns in javascript

1 Answer 1

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related