1
\$\begingroup\$

I'm just trying to learn how to write jQuery better so I wanted some smart peoples opinions.

The code is checking the billing and shipping address fields to see if it contains a PO Box address by looking for variations of "PO" and then displays a warning message after the input if it does contain it.

The code is duplicated for each one so there has to be a better way to write this.

jsFiddle

(function ($) {
    'use strict';

    // Check for PO Box text in address fields on checkout
    var $shipAddress = $('#checkout input[name="user_data[s_address]"]'),
        $billAddress = $('#checkout input[name="user_data[b_address]"]'),
        message = '<div class="message hidden"><p>We <strong>don\'t ship to PO Boxes</strong>. Sorry for the inconvenience.</p></div>';

    $shipAddress.after(message);
    $billAddress.after(message);

    $shipAddress.keyup(function () {
        var $value = $(this).val();

        if ($value === 'po' || $value === 'Po' || $value === 'PO' || $value === 'P.O' || $value === 'p.o') {
            $shipAddress.siblings('.message').removeClass('hidden');
        } else if ($value === '') {
            $shipAddress.siblings('.message').addClass('hidden');
        }
    });

    $billAddress.keyup(function () {
        var $value = $(this).val();

        if ($value === 'po' || $value === 'Po' || $value === 'PO' || $value === 'P.O' || $value === 'p.o') {
            $billAddress.siblings('.message').removeClass('hidden');
        } else if ($value === '') {
            $billAddress.siblings('.message').addClass('hidden');
        }
    });

}(jQuery));
\$\endgroup\$
2
\$\begingroup\$

There's 2 obvious places to clean this up a bit. The first is the duplicated keyup handler and the second is the check for PO.

Most jQuery functions can be called on a collection of elements. Since you are calling the same code on both your shipping address and billing address, we can combine them like so:

var $addresses = $('#checkout input[name="user_data[s_address]"]')
                 .add($('#checkout input[name="user_data[b_address]"]));

For the PO check, you can just use a regular expression:

//p, optional period, o
if(/p\.?o/i.test($value)) { ... }

So the whole thing becomes:

(function ($) {
    'use strict';

    // Check for PO Box text in address fields on checkout
    var $addresses = $('#checkout input[name="user_data[s_address]"]')
        .add($('#checkout input[name="user_data[b_address]"]')),
        message = "<div class='message hidden'><p>We <strong>don't ship to PO Boxes</strong>. Sorry for the inconvenience.</p></div>";

    $addresses.after(message);

    $addresses.keyup(function () {
        var $value = $(this).val();

        if(/p\.?o/i.test($value)) { 
            $(this).siblings('.message').removeClass('hidden');
        } else if ($value === '') {
            $(this).siblings('.message').addClass('hidden');
        }
    });

}(jQuery));

See: http://jsfiddle.net/6RnxG/7/

\$\endgroup\$
  • \$\begingroup\$ Thanks, that cleans it up a lot. Those were the exact spots I was looking to clean up. \$\endgroup\$ – ferne97 Feb 8 '13 at 2:16
1
\$\begingroup\$

In addition to drch's answser, this section

if(/p\.?o/i.test($value)) { 
    $(this).siblings('.message').removeClass('hidden');
} else if ($value === '') {
    $(this).siblings('.message').addClass('hidden');
}

could be rewritten as

var showMessage = ! /p\.?o/i.test($value);
 $(this).siblings('.message').toggleClass('hidden', showMessage);
\$\endgroup\$
1
\$\begingroup\$

It seems to be a good solution, BUT, what happens when the address is something like pollock road? It will throw an error.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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