current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite
1

I have got this working ok, I just think that it can be improved and reduce the amount of code. If anybody could help that would be brilliant. 

// Max Length Input
$('input[maxlength]').each(function() {         
    var maxCharLength = $(this).attr("maxlength");  

    if(maxCharLength < 0 || maxCharLength > 5000){
    } else {
        var charLength = $(this).val().length;
        $(this).after("<span class='charCount clearBoth' id='charCount_"+$(this).attr("name").replace(".","_")+"'> " + charLength + " of " + maxCharLength + " characters used</span>");        
        $(this).keyup(function() {              
            var charLength = $(this).val().length;              
            $(this).next("span").html(charLength + ' of ' + maxCharLength + ' characters used');

            if(charLength == maxCharLength ) {
                $(this).next("span").html('<strong> Maximum of ' + maxCharLength + ' characters used.</strong>');
                $(this).next("span").addClass("red");
            } else { $(this).next("span").removeClass("red"); }
        });
    }
});

// Max Length textarea
$('textarea[maxlength]').each(function() {          
    var maxCharLength = $(this).attr("maxlength");  

    if(maxCharLength < 0 || maxCharLength > 5000){
    } else {
        var charLength = $(this).val().length;
        $(this).after("<span class='charCount clearBoth'> " + charLength + " of " + maxCharLength + " characters used</span>");

            $('textarea[maxlength]').keyup(function(){

                var limit = parseInt($(this).attr('maxlength'));
                var text = $(this).val();
                var chars = text.length;

                $(this).next("span").html(chars + ' of ' + limit + ' characters used');

                if(chars > limit){
                    var new_text = text.substr(0, limit);
                    $(this).val(new_text);

                    $(this).next("span").html('<strong> Maximum of ' + limit + ' characters used.</strong>');
                    $(this).next("span").addClass("red");
                } else { $(this).find("span").removeClass("red"); }
            });
    }
});
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

There are a few things I would suggest:
1. Replace you numbers with constants and revert the condition to get rid of 'else':

var minTextLength = 0;
var maxTextLength = 5000;
if (maxCharLength >= minTextLength || maxCharLength <= maxTextLength){
      //Your code...
    }

2. Move the block of code:

$(this).next("span").html('<strong> Maximum of ' + maxCharLength + ' characters used.</strong>');
            $(this).next("span").addClass("red");
        } else { $(this).next("span").removeClass("red"); }

To a separate function.

And there are a couple of things that confuse me:
1. In 'Max Length textarea' you have:

$('textarea[maxlength]').keyup()

Is it correct? Becasue it means that you'll add a lot of handlers to all your textarea[maxlength].

2 . In your 'Max Length Input' you have:

if(charLength == maxCharLength )

Maybe it could be replaced with:

if(charLength > maxCharLength )

Depending on the answers to these questions, maybe it'll be possible to move the keyup handler to a separate function. And maybe even the whole code from both 'Max Length textarea' and 'Max Length Input'.

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.