Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question

2 Answers

up vote 3 down vote accepted

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();
    }
});
share|improve this answer
wow, thanks, great answer =D – user1606963 Sep 6 '12 at 9:01
$('#monhpf,#monmpf').keyup(function()
{
   var mon = $(this);
   var monValue = mon.val();
   // continue using mon
});

Warning: Not Tested!

share|improve this answer
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. – Jeff Vanzella Sep 5 '12 at 18:19

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.