10
\$\begingroup\$

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?

\$\endgroup\$
0

1 Answer 1

4
\$\begingroup\$

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');
});
\$\endgroup\$
2
  • \$\begingroup\$ can u explain more about this ? " The last function call is improperly closed, so technically that's broken. " . tnx \$\endgroup\$
    – Danin Na
    Sep 29, 2015 at 13:03
  • \$\begingroup\$ It should be }); not }};. \$\endgroup\$
    – ferada
    Sep 29, 2015 at 13:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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