current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

I wrote the following code using jQuery to create a WebAlert instead of using the alert() function from jQuery:

(function($){
    $.fn.WebAlert = function(msg) {
        // If WebAlert is Availble -> First Remove  this-> callback = Yes -> Now Create New WebAlert
        if ($('.WebAlert_Area').length) {
            $(this).Remove_WebAlert(msg , 'Yes')
            return false;
        //If WebAlert Not Availble  
        } else {
            $(this).Create_WebAlert(msg)
            return false;
        }
    }

    //Function For Create WebAlert
    $.fn.Create_WebAlert = function(msg) {
        $('body').append('<div class="WebAlert_Area"><div class="Msg_Area"></div></div>');
        $('.WebAlert_Area .Text_Area').html(msg)
    };

    //Function For Remove WebAlert
    $.fn.Remove_WebAlert = function(msg , callback) {
        $('.WebAlert_Area').hide(1000 , function() { 
            $(this).remove(); 
            if (callback == 'Yes') { 
                $(this).Create_WebAlert(msg)  
            }
        })  
    };
})(jQuery);

$(document).ready(function() {
    $('body').WebAlert('Welcome To My Web')
}};

But I think that it's too complex and non-standard, is that true?

share|improve this question
    
Probably a copy/paste error, but (document) should be $(document). –  Matt Zeunert yesterday
    
Does this code actually work? "Yes" is passed into Remove_WebAlert as the type parameter, but then you compare callback to "Yes". –  Matt Zeunert yesterday
    
both of top error are text problem and saloved . –  Danin Na yesterday

1 Answer 1

up vote 4 down vote accepted

Consistency and correct syntax would be a start: The last function call is improperly closed, so technically that's broken. Also my JavaScript mode marks all missing semicolons, so I inserted them too. The naming convention is unusual in the sense that it's mixing camel case and underscores, doesn't loook the greatest. The comments mostly state what the code already says - I'd leave them out unless they add significant insights.

Now to the actual code. WebAlert can be simplified since it always returns false.

The name callback implies the parameter being a function, which it's not, a better name could be inCallback or so?

The whole adding/removing nodes just for an alert seems messy. I'd say that you should always have the div for WebAlert_Area in the website, then toggle its visibility with CSS unless there's a pressing reason to do it this way.

In any case, removing the div just to recreate it immediately doesn't strike me as a good way. Can't it just stay there and be filled with new content (.html(msg)) instead of Remove_WebAlert(); Create_WebAlert?

Of course it is untested, but I think splitting it like below makes more sense and is less convoluted:

(function($){
    $.fn.WebAlert = function(msg) {
        if (!$('.WebAlert_Area').length) {
            $(this).Create_WebAlert();
        }
        $(this).Set_WebAlert(msg);
        return false;
    };

    $.fn.Create_WebAlert = function(msg) {
        $('body').append('<div class="WebAlert_Area"><div class="Msg_Area"></div></div>');
    };

    $.fn.Set_WebAlert = function(msg) {
        $('.WebAlert_Area .Text_Area').html(msg);
    };

    $.fn.Remove_WebAlert = function() {
        $('.WebAlert_Area').hide(1000, function() {
            $(this).remove();
        });
    };
})(jQuery);

$(document).ready(function() {
    $('body').WebAlert('Welcome To My Web');
});
share|improve this answer
    
can u explain more about this ? " The last function call is improperly closed, so technically that's broken. " . tnx –  Danin Na yesterday
    
It should be }); not }};. –  ferada yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.