Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How would you write this jQuery code cleaner and better? I'm a beginner.

$.extend({
misTip : function($tipSettings) {
    $tip = $tipSettings.tip ? $tipSettings.tip : '';
    $closeTime = $tipSettings.closeTime ? $tipSettings.closeTime : 1500;
    $refresh = $tipSettings.refresh;

    delete $tipSettings.msg;
    delete $tipSettings.closeTime;
    delete $tipSettings.refresh;

    //dialog ui tip
    var tpl = '';
    tpl += '<div style="padding:5px 20px">';
    tpl += '<p style="font-size:14px;padding-top:10px;"><span class="ui-icon ui-icon-alert" style="float:left; margin:0 7px 20px 0;"></span><span class="ajaxMsg">' + $tip + '</span></p>';
    tpl += '</div>';

    var $defaultTipSettings = {
        title : 'notice',
        slow : 'slide',
        width : 320,
        open : function (event, ui) {
            $(this).bind("keypress",function(event){
                $(this).dialog('close');
            });
            $dialog = $(this);
            setTimeout(function(){
                $dialog.dialog('close');
                if ($refresh) {
                    _refresh();
                }
            }, $closeTime);
        }
    }
    var $tipSettings = $.extend($defaultTipSettings, $tipSettings);
    $(tpl).dialog($tipSettings);
}
});
share|improve this question

1 Answer 1

up vote 1 down vote accepted

You are using several global variables in the code, which should be local.

You are prefixing variable names with $ for no apparent reason, that only makes the code harder to read.

You can use the || operator instead of the conditional operator to check for missing values.

You are using the variable _refresh in the code, I assume that it should be the variable that you defined instead.

$.extend({
  misTip : function(tipSettings) {
    var tip = tipSettings.tip || '';
    var closeTime = tipSettings.closeTime || 1500;
    var refresh = tipSettings.refresh;

    delete tipSettings.msg;
    delete tipSettings.closeTime;
    delete tipSettings.refresh;

    //dialog ui tip
    var tpl =
      '<div style="padding:5px 20px">' +
      '<p style="font-size:14px;padding-top:10px;"><span class="ui-icon ui-icon-alert" style="float:left; margin:0 7px 20px 0;"></span><span class="ajaxMsg">' + tip + '</span></p>' +
      '</div>';

    var defaultTipSettings = {
      title : 'notice',
      slow : 'slide',
      width : 320,
      open : function (event, ui) {
        $(this).bind("keypress",function(){
          $(this).dialog('close');
        });
        var dialog = $(this);
        setTimeout(function(){
          dialog.dialog('close');
          if (refresh) {
            refresh();
          }
        }, closeTime);
      }
    }
    var settings = $.extend(defaultTipSettings, tipSettings);
    $(tpl).dialog(settings);
  }
});
share|improve this answer
    
Thank you~ The way for my jQuery is so long~ –  bloody numen May 27 '13 at 11:00

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.