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.

I created a jQuery code to automatically insert new input field when the previous field is focused. The code works well but seems rather lengthy to me.

What I'm currently doing is cloning an element and then modifying clone's attributes. As I add new element into DOM, I also need to modify this element:

<input type="hidden" id="id_cat-TOTAL_FORMS" value="1" name="cat-TOTAL_FORMS">

and increase its value attribute by 1.

How would you refactor the jQuery code below?

jQuery:

var slist = {
    'init': function () {
        $('form#cat-form').on('focus', 'p:last input[type="text"]', slist.addCategoryForm);
    },

    'addCategoryForm': function() {
        slist.totalcatforms = parseInt($('form#cat-form input[id$=TOTAL_FORMS]').attr('value'));
        console.log(slist.totalcatforms);
        $('form#cat-form input[id$=TOTAL_FORMS]').attr('value', slist.totalcatforms + 1);
        console.log('TOTAL_FORMS = ' + parseInt($('form#cat-form input[id$=TOTAL_FORMS]').attr('value')));
        $('form#cat-form p:first').clone().insertBefore('form#cat-form input[type="submit"]');
        $('form#cat-form p:last label').attr('for', 'id_cat-' + slist.totalcatforms + '-name');
        $('form#cat-form p:last input[type="text"]').val('').attr({
            id: 'id_cat-' + slist.totalcatforms + '-name',
            name: 'cat-' + slist.totalcatforms + '-name'
            });
        $('form#cat-form p:last input[type="hidden"]').attr({
            id: 'id_cat-' + slist.totalcatforms + '-id',
            name: 'cat-' + slist.totalcatforms + '-id'
            });

    }
}

$(document).ready(slist.init);

HTML:

<form id="cat-form" method="post" action="/" name="cat-form">
<div style="display:none">
  <input type="hidden" value="hjYpU5a6AD7VxhZpSSQv3s3efJ7uh2Z8" name=
  "csrfmiddlewaretoken">
</div>

<input type="hidden" id="id_cat-TOTAL_FORMS" value="1" name=
"cat-TOTAL_FORMS">
<input type="hidden" id="id_cat-INITIAL_FORMS" value="0" name=
"cat-INITIAL_FORMS">
<input type="hidden" id="id_cat-MAX_NUM_FORMS" name=
"cat-MAX_NUM_FORMS">

<p>
    <label for="id_cat-0-name">Name:</label>
    <input type="text" maxlength="30" name=
"cat-0-name" id="id_cat-0-name">
    <input type="hidden" id="id_cat-0-id" name=
"cat-0-id">
</p>
<input type="submit" value="Save" name="catForm">
</form>
share|improve this question
add comment

2 Answers 2

Here's a quick review of your JavaScript code:

  1. Please remember to always cache your selectors.

  2. To get the value of an input element, use .val() instead of .attr('value').

  3. There's no need to use two separate steps to first get the value, then set it. Instead, pass a function to val, and return the new value you want.

  4. To convert a string to a number, just prefix it with the plus sign (e.g. +'32' will return the number 32). It's safer, and more concise, than parseInt.

  5. When using CSS selectors, you should always strive to use the native CSS3 selectors, since they're much faster than jQuery's own custom selectors. :first and :last are not CSS3 selectors. In your case, you should use :first-of-type and :last-of-type, since they're native CSS3 selectors.

  6. Since IDs are unique per page, there's no reason to qualify an ID selector with the tag name (as you've done with form#cat-form). Just use the ID on its own. Again, better performance.


With all that in mind, here's some sample code for you:

var slist = {
    cache: {},
    init: function () {
        slist.cache.form        = $('#cat-form');
        slist.cache.totalsInput = slist.cache.form.find('input[id$=TOTAL_FORMS]');
        slist.cache.toClone     = slist.cache.form.find('p:first-of-type');

        slist.cache.form.on('focus', 'p:last-of-type input[type="text"]', slist.addCategoryForm);
    },
    addCategoryForm: function() {

        var totalCount;
        slist.cache.totalsInput.val(function (i, v) {
            totalCount = +v;
            return totalCount++;
        });

        slist.cache.toClone.clone()

            .find('label')
                .prop('for', 'id_cat-' + totalCount + '-name')

            .end().find('input[type="text"]')
                .val('').prop({
                    id: 'id_cat-' + totalCount + '-name',
                    name: 'cat-' + totalCount + '-name'
                })

            .end().find('input[type="hidden"]')
                .prop({
                    id: 'id_cat-' + totalCount + '-id',
                    name: 'cat-' + totalCount + '-id'
                })

            .end().insertAfter( $(this).closest('p') );
    }
}

$(slist.init);
share|improve this answer
add comment

Here's a short demo, and the code is commented to reflect my take on improvements

HTML

<form id="cat-form" method="post" action="/" name="cat-form">
    <!--
    our template
    this is fetched as a string, and converted into HTML by jQuery later on
    this does not execute since the type "text/template" is unknown to the browser
    -->
    <script type="text/template" id="nameTemplate">
        <li>
            <input type="hidden" id="id_cat-{name}-id" name="cat-{name}-id">
            <label for="id_cat-{name}-name">Name:</label>
            <input type="text" id="id_cat-{name}-name" name="cat-{name}-name" maxlength="30">
        </li>
    </script>
    <div style="display:none">
        <input type="hidden" value="hjYpU5a6AD7VxhZpSSQv3s3efJ7uh2Z8" name="csrfmiddlewaretoken">
    </div>
    <!-- 
    i would use IDs rarely, only when elements are unique.
    for form elements, i would stick to names instead
    -->
    <input type="hidden" name="cat-TOTAL_FORMS" value="0">
    <input type="hidden" name="cat-INITIAL_FORMS" value="0">
    <input type="hidden" name="cat-MAX_NUM_FORMS">
    <!--
    create a container for all the names
    since they are somewhat related to eachother, we use a list
    we also avoid using insertBefore(submitButton)
    -->
    <ul class="id_cat-names"></ul>
    <input type="submit" value="Save" name="catForm">
</form>​​​

JS

$(function() {

        //IDs are unique. No need to be so specific on the selector.
        //we take form as our "base" to find the other elements.  
    var form = $('#cat-form'),
        //look for the name list in context of the form
        //this totally works like form.find('.id_cat-names'), only shorter
        nameList = $('.id_cat-names',form),
        //cache the total forms element, finding it using the same method
        totalForms = $('input[name="cat-TOTAL_FORMS"]', form), 
        //we get the template text
        //this is very useful for creating multiple elements of the same internals
        template = $('#nameTemplate', form).html(), 
        //track latest count, initially based on totalForms value
        latestCount = totalForms.val();

    function addCategoryForm(){
        //we then take the template text
        //replace the numbers with an incremented count
        //turn it into HTML using jQuery
        //append it to nameList
        $(template.replace(/\{name\}/g,++latestCount)).appendTo(nameList);
        //update latest count
        totalForms.val(latestCount);
    }

    //focus event handler for the last name field
    //quotes are optional on single word element types
    nameList.on('focus', 'li:last input[type=text]', function(){
        addCategoryForm();
    });

    addCategoryForm();

}); ​
share|improve this answer
    
Very interesting! A lot of new things for me in your approach. Thanks, I really appreciate your feedback! I think I won't be able to use it "as is" because I can't modify the HTML output that much. A lot of it is generated by the framework (Django). For example Django relies on element ids when processing forms, so they must stay. In any case, your response is fantastic for learning purposes. Thanks again. –  finspin Dec 2 '12 at 19:08
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.