0
\$\begingroup\$

first here is the code:

Part 1
    $('#monhpf').keyup(function()
    {
        /* General variable(s) */
        var monhpf = $('#monhpf').val();

        if(monhpf=="")
        {
            $('#monhpf').css('border-color',red);
            $('.mondayErrors').text(emptyField).show();
        }else       
        if(monhpf.length > 0)
        {
            /* Verifies the charset of the zip code */
            $.post('functions/hoursCharset.php',{hoursPHP:monhpf},function(hoursCharset)
            {
                if(hoursCharset==true)
                {
                    /* Charset valid*/
                    $('#monhpf').css('border-color',green);
                    $('.mondayErrors').hide();
                    mondayErrors = false;
                }else
                {
                    /* Charset not valid */
                    $('#monhpf').css('border-color',orange);
                    $('.mondayErrors').text(invalid).show();
                    mondayErrors = true;
                }
            });
        }
    });
Part 2  
    $('#monmpf').keyup(function()
    {
        /* General variable(s) */
        var monmpf = $('#monmpf').val();

        if(monmpf=="")
        {
            $('#monmpf').css('border-color',red);
            $('.mondayErrors').text(emptyField).show();
        }else       
        if(monmpf.length > 0)
        {
            /* Verifies the charset of the zip code */
            $.post('functions/minutesCharset.php',{minutesPHP:monmpf},function(minutesCharset)
            {
                if(minutesCharset==true)
                {
                    /* Charset valid*/
                    $('#monmpf').css('border-color',green);
                    $('.mondayErrors').hide();
                    mondayErrors = false;
                }else
                {
                    /* Charset not valid */
                    $('#monmpf').css('border-color',orange);
                    $('.mondayErrors').text(invalid).show();
                    mondayErrors = true;
                }
            });
        }
    });

As you can see part 1 and part 2 are pretty much the same except for the #monhpf and #monmpf variables. My question is, how can I improve this code to be smaller? Like in a single function for example.

\$\endgroup\$
3
\$\begingroup\$

1)

Take advantage of javascript's truthy and falsey values.

Boolean(undefined); // => false
Boolean(null); // => false
Boolean(false); // => false
Boolean(0); // => false
Boolean(""); // => false
Boolean(NaN); // => false

Boolean(1); // => true
Boolean([1,2,3]); // => true
Boolean(function(){}); // => true

More here: http://james.padolsey.com/javascript/truthy-falsey/

if(monmpf == "") => if(!monmpf).

if(minutesCharset == true) => if(minutesCharset)

if(monhpf.length > 0) => if( monhpf.length )

2)

You can refer to the current element by using $(this).

$('#monmpf').keyup(function () {
// el refers to $("monmpf")
var el = $(this);

3)

if (monhpf.length > 0) is redundant since you already checked the string value with this, if(monmpf == "").

4)

You're using too many global variables, such as emptyField, invalid, green, red, orange, etc. Try wrapping the variables inside a meaningful namespace, like

var COLORS = { red: "red", green: "green" };
var MESSAGE = { invalid: "Wrong", emptyField: "" };

5)

mondayErrors? I think you should rename this variable.

6)

Break up functions that are longer than 8-12 lines of code into smaller ones.

With all that said, here's what I came up with.

var getRequestObject = function (name, value) {
    var obj = {};
    switch (name) {
        case "monhpf":
            obj.url = 'functions/hoursCharset.php';
            obj.paramObj = {
                hoursPHP : value
            };
            break;
        case "monmpf":
            obj.url = 'functions/minutesCharset.php';
            obj.paramObj = {
                minutesPHP : value
            };
            break;
    }
    return obj;
};
var getPostFunc = function ($el) {
    return function (isCharset) {
        mondayErrors = !isCharset;
        if (isCharset) {
            $el.css('border-color', green);
            $('.mondayErrors').hide();
        } else {
            $el.css('border-color', orange);
            $('.mondayErrors').text(invalid).show();
        }
    };
};
$('#monhpf, #monmpf').keyup(function () {
    var $el = $(this),
        request,
        value = $el.val(),
        id = $el.attr('id');

    if (value) {
        request = getRequestObject(id, value);
        $.post(request.url, request.paramObj, getPostFunc($el));
    } else {
        $el.css('border-color', red);
        $('.mondayErrors').text(emptyField).show();
    }
});
\$\endgroup\$
0
\$\begingroup\$
$('#monhpf,#monmpf').keyup(function()
{
   var mon = $(this);
   var monValue = mon.val();
   // continue using mon
});

Warning: Not Tested!

\$\endgroup\$
  • \$\begingroup\$ Can you elaborate on your code snippet a bit. A description of what you are doing and why you decided to do it would be very helpful. \$\endgroup\$ – Jeff Vanzella Sep 5 '12 at 18:19

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.