How to keep data functions and variables DRY?

Question

How do I make this Jquery dry?

// Adding data to input depending on what div is clicked 

    $('#navninfoDiv').click(function() {
        $('input#webhost_navninfo').data('myData', null);
    });
    $('#navninfoDivP').click(function() {
        $('input#webhost_navninfo').data('myData', 'p');
    });
        $('navninfoDivV').click(function() {
        $('#inputwebhost_navninfo').data('myData', 'V');
    });

// Adding data to input depending on what div is clicked 

    $('#prisinfoDiv').click(function() {
        $('#inputwebhost_prisinfo').data('myData2', null);
    });
    $('#prisinfoDivP').click(function() {
        $('#inputwebhost_prisinfo').data('myData2', 'p');
    });
        $('#prisinfoDivV').click(function() {
        $('#inputwebhost_prisinfo').data('myData2', 'V');
    });

// Adding data to input depending on what div is clicked 


    $('#cableinfoDiv').click(function() {
        $('#inputwebhost_cableinfo').data('myData3', null);
    });
    $('#cableinfoDivP').click(function() {
        $('#inputwebhost_cableinfo').data('myData3', 'p');
    });
        $('#prisinfoDivV').click(function() {
        $('#inputwebhost_cableinfo').data('myData3', 'V');
    });

// Adding data to input on submit 
    $('#smt').click(function() {

// for input#webhost_navninfo

        var myData = $('#webhost_navninfo').data('myData'),
            val = $('#webhost_navninfo').val();
        if (myData) {
            $('#webhost_navninfo').val(myData + val);
        }

// for input#webhost_prisinfo

        var myData2 = $('#webhost_prisinfo').data('myData2'),
            val2 = $('#webhost_prisinfo').val();
        if (myData2) {
            $('#webhost_prisinfo').val(myData2 + val2);
        }

// for input#webhost_cableinfo

        var myData3 = $('#webhost_cableinfo').data('myData3'),
            val3 = $('#webhost_cableinfo').val();
        if (myData3) {
            $('#webhost_navninfo').val(myData3 + val3);
        }
    });

How do I dry all this code up? I have many more input fields about 50.

Here is my HTML and jQuery without the data functions: http://jsfiddle.net/z5qeX/2/

The code example I gave in this example is just to illustrate the functionality I want. It does not match the HTML in the jsfiddle, but it is the code I want to add it to later.

Answer 1

This is a sytematic refinement of the code which you provided. There are alternate solutions (like ShankarSangoli's) which provide you with good alternatives that require modifications to the HTML, but this should work with exactly what you have right now.

This is going to be done in multiple passes so I can explain what is going on.

// pass 1 you have three sets of information, the difference is the 
// div which has the listener, and the default data.  (I've taken the liberty to
// make all of the names consistent.
$('#navninfoDiv').click(function() {
    // based on your HTML, I believe that the input# is superfluous
    $('#webhost_navninfo').data('myData', null);
});
$('#navninfoDivP').click(function() {
    $('#webhost_navninfo').data('myData', 'p');
});
$('navninfoDivV').click(function() {
    $('#webhost_navninfo').data('myData', 'V');
});

$('#prisinfoDiv').click(function() {
    $('#webhost_prisinfo').data('myData2', null);
});
/* ... */

$('#cableinfoDiv').click(function() {
    $('#webhost_cableinfo').data('myData3', null);
});
/* ... */

Since you're using a naming convention, it is actually pretty simply to loop that. First we'll place the "data" part in a loop:

var defaults = [null, "P", "V"]
for( var i = 0; i < defaults.length; i++ )
{
    var divID = '#navninfoDiv' + ( defaults[i]?defaults[i]:"" );
    $(divID).click(function() {
        $('#webhost_navninfo').data('myData', defaults[i]);
    });
}

for( i = 0; i < defaults.length; i++ )
{
    var divID = '#prisinfoDiv' + ( defaults[i]?defaults[i]:"" );
    $(divID).click(function() {
        $('#webhost_prisinfo').data('myData2', defaults[i]);
    });
}

for( i = 0; i < defaults.length; i++ )
{
    var divID = '#cableinfoDiv' + ( defaults[i]?defaults[i]:"" );
    $(divID).click(function() {
        $('#webhost_cableinfo').data('myData3', defaults[i]);
    });
}

Based on that, it looks like we have some repeaded code still, what if we looped over the divs' ID's too?

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
var defaults = [null, "P", "V"]
for( var j = 0; j < baseIDs.length; j++ )
{
    for( var i = 0; i < defaults.length; i++ )
    {
        var divID = '#'+baseIDs[j]+'Div' + ( defaults[i]?defaults[i]:"" );
        $(divID).click(function() {
            var dat = i? 'myData': 'myData' + String( i + 1 );
            $('#webhost_'+baseIDs[j]).data(dat, defaults[i]);
        });
    }
}

That doesn't look very clean. I bet it wouldn't be hard to take all of that center stuff and put it into its own little function which would make it far easier to debug and a lot cleaner:

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
var defaults = [null, "P", "V"]
for( var j = 0; j < baseIDs.length; j++ )
{
    for( var i = 0; i < defaults.length; i++ )
    {
        var datLab = 'myData'
        if( i ) datLab += String( i + 1 );
        assignClickListener(baseIDs[j], defaults[i], datLab);
    }
}

function assignClickListener(base, defaults, datLab)
{
    var divID = '#'+base+'Div' + ( defaults?defaults:"" );
    $(divID).click(function() {
        $('#webhost_'+base).data(datLab, defaults);
    });
}

So, now we've finished the first part of the script. Let's see if there is a similar way to look at the second part of your script.

// I'm going to omit this until we re-unite all of the code at the end.
$('#smt').click(function() {
    // so you have the same looping structure here:
    var myData = $('#webhost_navninfo').data('myData'),
        val = $('#webhost_navninfo').val();
    if (myData) {
        $('#webhost_navninfo').val(myData + val);
    }

    var myData2 = $('#webhost_prisinfo').data('myData2'),
        /* ... */

    var myData3 = $('#webhost_cableinfo').data('myData3'),
        val3 = $('#webhost_cableinfo').val();
    if (myData3) {
        // this looks like a mistake. I will assume that it is supposed
        // to be webhost_cableinfo.
        $('#webhost_navninfo').val(myData3 + val3);
    }
});

Well, since the loop worked so well last time, why don't we try to do the same thing again?

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
for( var i = 0; i < baseIDs.length; i++ )
{
    var datLab = 'myData'
    if( i ) datLab += String( i + 1 );
    var currentBase = baseIDs[i];
    var myData = $('#webhost_'+currentBase).data(datLab),
        val = $('#webhost_'+ currentBase).val();
    if (myData) {
        $('#webhost_' + currentBase).val(myData + val);
    }
}

Whoops. Looks like I missed somethign there -- I could re-use that ID:

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
for( var i = 0; i < baseIDs.length; i++ )
{
    var datLab = 'myData'
    if( i ) datLab += String( i + 1 );
    // since this is actually the ID now, I've renamed the variable.
    var currentID = '#webhost_'+baseIDs[i];
    var myData = $(currentID).data(datLab),
        val = $(currentID).val();
    if (myData) {
        $(currentID).val(myData + val);
    }
}

Hmmm... I bet I could cache even more of that:

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
for( var i = 0; i < baseIDs.length; i++ )
{
    var datLab = 'myData'
    if( i ) datLab += String( i + 1 );
    // I can actually cache this entire element. I'll do that:
    var currentElem = $('#webhost_'+baseIDs[i]);
    var myData = currentElem.data(datLab),
        // val = currentElem.val(); <!-- I don't need this value, I'll just
        // calculate it inline.
    if (myData) {
        currentElem.val(myData + currentElem.val());
    }
}

Ok, now we're ready to combine both and see how we've done:

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
var defaults = [null, "P", "V"]
for( var j = 0; j < baseIDs.length; j++ )
{
    for( var i = 0; i < defaults.length; i++ )
    {
        var datLab = 'myData'
        if( i ) datLab += String( i + 1 );
        assignClickListener(baseIDs[j], defaults[i], datLab);
    }
}

function assignClickListener(base, defaults, datLab)
{
    var divID = '#'+base+'Div' + ( defaults?defaults:"" );
    $(divID).click(function() {
        $('#webhost_'+base).data(datLab, defaults);
    });
}

for( i = 0; i < baseIDs.length; i++ )
{
    var datLab = 'myData'
    if( i ) datLab += String( i + 1 );
    var currentElem = $('#webhost_'+baseIDs[i]);
    var myData = currentElem.data(datLab),
    if (myData) {
        currentElem.val(myData + currentElem.val());
    }
}

Notice the bug? Notice places to remove duplicate code?

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
var defaults = [null, "P", "V"]
for( var j = 0; j < baseIDs.length; j++ )
{
    for( var i = 0; i < defaults.length; i++ )
    {
        /*
        var datLab = 'myData'
        if( i ) datLab += String( i + 1 );
        I really don't like this pattern. I repeat it twice. We can fix that
        */
        assignClickListener(baseIDs[j], defaults[i], getDataLabel(j));
    }
}

// I left this part out. That's the bug.
$('#smt').click(function() {
    // make i local here. Otherwise it could be destructive
    for( var i = 0; i < baseIDs.length; i++ )
    {
        var currentElem = $('#webhost_'+baseIDs[i]);
        var myData = currentElem.data(getDataLabel( i )),
        if (myData) {
            currentElem.val(myData + currentElem.val());
        }
    }
}

function assignClickListener(base, defaultData, datLab)
{
    var divID = '#'+base+'Div' + ( defaultData?defaultData:"" );
    $(divID).click(function() {
        $('#webhost_'+base).data(datLab, defaultData);
    });
}

function getDataLabel( i )
{
    var datLab = 'myData'
    if( i ) datLab += String( i + 1 );
    return datLab;
}

I can see one last thing which can be removed easily. We keep looking up $('#webhost_<id>'). What if that needs to change? No, I think we should move that out as well. While we're at it, we might as well cache it:

var baseIDs = [ 'navninfo', 'prisinfo', 'cableinfo' ];
var defaults = [null, "P", "V"]
for( var j = 0; j < baseIDs.length; j++ ) {
    for( var i = 0; i < defaults.length; i++ ) {
        assignClickListener(baseIDs[j], defaults[i], getDataLabel(j));
    }
}

$('#smt').click(function() {
    for( var i = 0; i < baseIDs.length; i++ ) {
        var currentElem = getHostElement(baseIDs[i]);
        var myData = currentElem.data(getDataLabel( i )),
        if (myData) {
            currentElem.val(myData + currentElem.val());
        }
    }
} ); // note: this was a bug in previous iteration. I missed the );

function assignClickListener(base, defaultData, datLab) {
    var divID = '#'+base+'Div' + ( defaultData?defaultData:"" );
    $(divID).click(function() {
        getHostElement(base).data(datLab, defaultData);
    });
}

var elementCache = {}
function getHostElement( baseLab ) {
    if( !elementCache[ baseLab ] ) 
        elementCache[ baseLab ] = $('#webhost_'+baseLab);
    return elementCache[ baseLab ];
}

function getDataLabel( i ) {
    var datLab = 'myData'
    if( i ) datLab += String( i + 1 );
    return datLab;
}

Now, the result is only about 50% fewer lines of code and it is far easier to test and because it has less direct repetition, each piece of this code is easier to understand.

Answer 2

It's a good idea to only define all those literals once.

var divP = 'P';
var divV = 'v';

Doing the same with DOM element references spares jQuery from identifying the element again and again. So it also runs faster.

var webHostNavInfo = $('#webhost_navninfo');
var inputWebHostNavInfo = $('#inputwebhost_navninfo');

Answer 3

You could try:

var config = { 
    "prefix" : { 
        "navinfo": "", 
        "prisinfo": "2", 
        "cableinfo": "3"
    },
    "map" : { 
       "": null, // Not a 100% sure about this. Worked on Chrome for me, but should try it in IE see if it works or chokes.
       "P": "p",
       "V": "v"
    }
 };

 for (section in config['prefix']) {
   for (m in config['map']) {
     $('#' + section+ 'Div' + m ).click(function() {
        $('input#webhost_navninfo').data('myData' + config['prefix'][section], null);
     });
   }
 } 

and do something similar for the rest of it. In the $("#smt").click bit.

Answer 4

Below is a first pass. I'm not sure how far you want to take this, but as arunkumar points out, it could all be data-driven.

A few questions that might help:

• do you need different data keys for each of these data values? If not, this can be compressed down.
• can you use classes (instead of IDs) to identify these elements? If so, everything can be instrumented at once and (perhaps) the loop eliminated.
• can you name this behavior you are instrumenting here, and encapsulate it into a plugin? If so, that will clarify the code and causal relationships. I don't understand whta navninfo vs. prisinfo vs. cableinfo is, so I can't do anything logically to share the code.

What follows is smaller code. It can certainly be dried up more (a function that creates the click handler anyone?), but without understanding the context a bit, I'm hesitant to do so:

var infoDivs = ['navninfo', 'prisinfo', 'cableinfo'];

// Adding data to input depending on what div is clicked 
$.each(infoDivs, function(index, divBaseName) {
    var key = 'myData' + (index + 1);
    var $elem = $('#inputwebhost_' + divBaseName);
    $('#' + divBaseName).click(function() {
        $elem.data(key, null);
    });
    $('#' + divBaseName + 'P').click(function() {
        $elem.data(key, 'p');
    });
    $('#' + divBaseName + 'V').click(function() {
        $elem.data(key, 'v');
    });
});

$('#smt').click(function() {
    $.each(infoDivs, function(index, divBaseName) {
        var $elem = $('#webhost_' + divBaseName);
        var myData = $elem.data('myData' + (index + 1));
        var val = $elem.val();
        if(myData) {
            $elem.val(myData + val);
        }
    });
});

Answer 5

Your code seemed to have some inconsistencies, so I made a few assumptions. For example, I'm not sure if you always meant $('input#webhost_navninfo') or $('#inputwebhost_navninfo').

var infoDivs = ['navninfo', 'prisinfo', 'cableinfo'];

// Adding data to input depending on what div is clicked 
$.each(infoDivs, function(index, divBaseName) {
    var dataIndex = index + 1;
    $('#' + divBaseName).click(function() {
        $('#inputwebhost_' + divBaseName).data('myData' + dataIndex, null);
    });
    $('#' + divBaseName + 'P').click(function() {
        $('#inputwebhost_' + divBaseName).data('myData' + dataIndex, 'p');
    });
    $('#' + divBaseName + 'V').click(function() {
        $('#inputwebhost_' + divBaseName).data('myData' + dataIndex, 'v');
    });
});

$('#smt').click(function() {
    $.each(infoDivs, function(index, divBaseName) {
        var myDataIndex = index + 1;
        var elem = $('#webhost_' + divBaseName);
        var myData = elem.data('myData' + myDataIndex);
        var val = elem.val();
        if(myData) {
            elem.val(myData + val);
        }
    });
});

If you don't care about how myData is numbered so long as the information is all associated correctly, then you could get rid of the variable dataIndex and just use index in both cases.

Answer 6

The idea here is to have minimal changes to the markup as well as achieve the goals with minimal code.

Assuming that you have markup like:

<div id="navninfoDiv">navninfo</div>
<div id="navninfoDivP">navninfop</div>
<div id="navninfoDivV">navninfoV</div>

<div id="prisinfoDiv">prisinfo</div>
<div id="prisinfoDivP">prisinfop</div>
...

I rearranged it within a container div to:

• take advantage of the very efficient .delegate()
• make the selection code little easier to read

HTML:

 <div id="container">
    <div id="navninfoDiv">navninfo</div>
    <div id="navninfoDivP">navninfop</div>
    <div id="navninfoDivV">navninfoV</div>

    <div id="prisinfoDiv">prisinfo</div>
    <div id="prisinfoDivP">prisinfop</div>
    <div id="prisinfoDivV">prisinfoV</div>

    <div id="cableinfoDiv">cableinfo</div>
    <div id="cableinfoDivP">cableinfop</div>
    <div id="cableinfoDivV">cableinfoV</div>
</div>

<input type="text" id="webhost_navninfo" />
<input type="text" id="webhost_prisinfo" />
<input type="text" id="webhost_cableinfo" />


<input type="button" id="smt" value="Submit" />

JS:

// if all we need is to access the data during submit, 
// we can store it in an object for faster access
var submitData = submitData || {};

$(function(){
    $('#container').delegate('div', 'click', function(){
        // figure out our delimiter
        var delimiter = 'Div',
            delimiterIndex = this.id.indexOf(delimiter);

        // extract type
        var type = this.id.substring(0, delimiterIndex);

        // extract data
        var data = this.id.substring(delimiterIndex + delimiter.length);

        // optional - uncomment to store null in place of empty string
        // data = (data == "") ? null : data;

        // assign data to appropriate type (navinfo, prisinfo etc.)
        // overwrite if already there
        submitData[type] = { selector: '#webhost_' + type, data: data};
    });


    $('#smt').click(function() {
        $.each(submitData, function(key, info){
           // get the input selector and its value
            var $inp = $(info.selector)
                val = $inp.val();

            // append to existing value
            $inp.val(info.data + val);
        })

        // comment out to perform submit    
        return false;
    });
});

Click for Live Demo !

Tried to keep it simple and readable and made all those assumptions. I hope this sets you up in the right direction.

I like Shankar's answer too simply because of use of data attributes. However, I don't like those pesky selectors. Plus too many calls to .data bag which is a overhead I tried to avoid.