How can I refactor this jQuery code?

Question

The code below is for a simple newsletter signup widget.

I'm sure there's a way to make it more concise, any ideas?

var email_form = $('.widget_subscribe form'); 
var email_submit = $('.widget_subscribe .submit');
var email_link = $('.widget_subscribe .email');

// Hide the email entry form when the page loads
email_form.hide();

// Show the form when the email link is clicked
$(email_link).click( function () {
    $(this).toggle();
    $(email_form).toggle();
    return false;
});

// Hide the form when the form submit is clicked
$(email_submit).click( function () {
    $(email_link).toggle();
    $(email_form).toggle();
});

// Clear/reset the email input on focus 
$('input[name="email"]').focus( function () {
    $(this).val("");
    }).blur( function () {
        if ($(this).val() == "") {
            $(this).val($(this)[0].defaultValue);
        }
});

Looks pretty good. You only miss ); after the last blur function. — Pim Jager
– Pim Jager, Commented May 5, 2009 at 13:48
Looks quite ok to me. You don't have to $() the variables like $(email_form) because that's been done when instatiating them. — Rashack
– Rashack, Commented May 5, 2009 at 13:50
As a sidenote... when I make a typo in my e-mail address and attempt to correct it your code will completely eradicate what I had typed before, forcing me to start over again. (This happens a LOT on websites and it sucks.) — Huppie
– Huppie, Commented Jul 1, 2009 at 13:00
you can drop the second and third var statement and just use ',' at the end. — Mark Rogers
– Mark Rogers, Commented Dec 3, 2009 at 16:27

Patrick McElhaney · Accepted Answer · 2011-12-22 13:32:08Z

You have some similar code here.

// Show the form when the email link is clicked
$(email_link).click( function () {
        $(this).toggle();
        $(email_form).toggle();
        return false;
});

// Hide the form when the form submit is clicked
$(email_submit).click( function () {
        $(email_link).toggle();
        $(email_form).toggle();
});

It could be refactored so the similarity is obvious.

// Show the form when the email link is clicked
$(email_link).click( function () {
        $(email_link).toggle();
        $(email_form).toggle();
        return false;
});

// Hide the form when the form submit is clicked
$(email_submit).click( function () {
        $(email_link).toggle();
        $(email_form).toggle();
});

So you could wrap toggling the link and the form into a function.

var toggleEmailLinkAndForm = function () {
        $(email_link).toggle();
        $(email_form).toggle();
}   

$(email_link).click(toggleEmailLinkAndForm);
$(email_submit).click(toggleEmailLinkAndForm);

And as others have pointed out, you can drop the redunant $()s.

var toggleEmailLinkAndForm = function () {
        email_link.toggle();
        email_form.toggle();
}   

email_link.click(toggleEmailLinkAndForm);
email_submit.click(toggleEmailLinkAndForm);

gregmac · Accepted Answer · 2009-05-05 13:48:49Z

3

It's already pretty concise, there's not much more you can do.

Anywhere you have $(email_submit) you can just have email_submit, because you've already wrapped it in $() (which makes it a jquery object).

Eg:

email_submit.click( function () {
    email_link.toggle();
    email_form.toggle();
});

answered May 5, 2009 at 13:48

gregmac

25.6k11 gold badges93 silver badges121 bronze badges

Comments

gradbot · Accepted Answer · 2009-05-05 14:34:02Z

2

I like Patrick McElhaney Code Best.

toggleEmailLinkAndForm() {
    email_link.toggle();
    email_form.toggle();
}   

email_link.click(toggleEmailLinkAndForm);
email_submit.click(toggleEmailLinkAndForm);

Part of refactoring is not going overboard. I would not recommend creating an extra click event that calls the other click event. The point of refactoring is readability and flexibility. You can also use the jQuery method "add" to shrink the code but it will become even harder to read.

email_link.add(email_submit).click(function(){
    email_link.add(email_form).toggle();
});

answered May 5, 2009 at 14:34

gradbot

13.9k5 gold badges41 silver badges69 bronze badges

Comments

Keith · Accepted Answer · 2009-05-05 14:16:24Z

1

Like Patrick said (+1), and you can also skip the extra function:

email_submit.click(function () {
    email_link.toggle();
    email_form.toggle();
});
email_link.click(function () {
    email_submit.click(); //calls the click function already subscribed
    return false;
});

answered May 5, 2009 at 14:16

Keith

157k82 gold badges314 silver badges453 bronze badges

Collectives™ on Stack Overflow

How can I refactor this jQuery code?

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related