How to start optimising/refactoring jQuery code

Question

I have following jQuery code

$('a.editpo, a.resetpass').click(function(event){
    event.preventDefault();
    var urlToCall = $(this).attr('href');
    var hyperlinkid = '#'+$(this).attr('id');
    var targetId = $(this).attr('id').match(/\d+/);
    var targetTrDiv = '#poformloadertr_'+targetId;
    var targetTdDiv = '#poformloadertd_'+targetId;
    var currentLink = $(this).html();
    /*Todo: refactor or shorten the following statement*/
    if((currentLink=='Edit' && $('#resetuserpassform_'+targetId).is(':visible')) 
        ||
       (currentLink=='Reset Pass' && $('#account-home-container-'+targetId).is(':visible'))
        ||
       ($(targetTdDiv).html() =='')
    ){
        $.ajax({
            url:urlToCall,
            success: function(html){
                $(targetTdDiv).html(html);
                $(targetTrDiv).show();
            }
        });
    }else{
        $(targetTrDiv).hide();
        $(targetTdDiv).html('');
    }
});

The editpo and resetpass are classes applied on hyperlinks in column of table, namely Edit and Reset Pass, clicking these load up the form in a table row, ids for the respective tr and td is targetTrDiv and targetTdDiv. I am not good at JS and specially jQuery, but would still be interested in any optimisations.

But I am especially interested in reducing the condition statement.

Diosney · Accepted Answer · 2011-08-18 16:25:21Z

1

First, you can optimize the following code:

   var urlToCall = $(this).attr('href');
   var hyperlinkid = '#'+$(this).attr('id');
   var targetId = $(this).attr('id').match(/\d+/);
   var targetTrDiv = '#poformloadertr_'+targetId;
   var targetTdDiv = '#poformloadertd_'+targetId;
   var currentLink = $(this).html();

to:

   var wrappedSet$ = $(this);

   var urlToCall = wrappedSet$.attr('href');
   var hyperlinkid = '#'+wrappedSet$.attr('id');
   var targetId = wrappedSet$.attr('id').match(/\d+/);
   var targetTrDiv = '#poformloadertr_'+targetId;
   var targetTdDiv = '#poformloadertd_'+targetId;
   var currentLink = wrappedSet$.html();

EDIT: Also, you could remove the currentLink=='Edit' && and currentLink=='Reset Pass' && code snippets, because you can be sure that the right links were clicked using the class selector that you are using in your jQuery click handler (a.editpo, a.resetpass).

If you do that the codition statement will stays like this:

if(($('#resetuserpassform_'+targetId).is(':visible')) 
        ||
       ($('#account-home-container-'+targetId).is(':visible'))
        ||
       ($(targetTdDiv).html() =='')
    ){
       /* AJAX call goes here */
    }

Besides, and hopefully I'm wrong, it's very unlikely that the condition statement can be more optimized. The reason for concluding this is that you want to much specificity that is likely impossible to achieve in other way.

Hope it helps you.

edited Aug 18, 2011 at 16:25

answered Aug 18, 2011 at 15:57

Diosney

10.6k15 gold badges70 silver badges113 bronze badges

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

2 Comments

Kumar Over a year ago

oh! how can I forget this, thanks for pointing out, code changed :-)

Kumar Over a year ago

I have to agree about the condition part, it can not be further optimised and the condition is if((link A is clicked and form for Link B is visible) OR (link B is clicked and form for Link A is visible) OR (3rd condition)) but thanks :-)

Collectives™ on Stack Overflow

How to start optimising/refactoring jQuery code

1 Answer 1

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related