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.

Just wanted some opinions on how to write this code a little better, it is working properly, I'm just trying to learn how to write jQuery better so I wanted some smart peoples opinions. Thanks!

It 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 duplicating for each one so there has to be a better way to write this.

http://jsfiddle.net/ferne97/6RnxG/

(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));
share|improve this question
    
Just so you know, there is a stack overflow question with a more complete PO box matching expression. It may help you out: stackoverflow.com/questions/5680050/… –  Danny Kirchmeier Feb 8 '13 at 5:35
add comment

2 Answers

up vote 2 down vote accepted

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/

share|improve this answer
    
Thanks, that cleans it up a lot. Those were the exact spots I was looking to clean up. –  ferne97 Feb 8 '13 at 2:16
add comment

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);
share|improve this answer
add comment

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.