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.

I know its a bit beyond what you have specifically asked, but if you will need to do this kind of coding repeatedly, my DRY sense of humor suggests that you consider using code snippets, code templates, and code generators to help you avoid having to repetitively type in the same absolutely necessary but uncomfortably similar code. To me, DRY is a higher level abstraction than mere internal code organization within a module. It's also important on the working process level. Good luck! — John Tobler, Aug 25 '11 at 16:07

ShankarSangoli · Answer 1 · 2011-08-24T13:51:12.503

Here is my simple solution with very less script provided you add couple of data attributes into all 3 types of div's as mentioned below. Also instead of using mydata, mydata2 and mydata3 we can just use mydata as data attribute name for all types of input.

Here is the working demo

Markup

<div id="navninfoDiv" data-type="navninfo" data-info="">navninfo</div>
<div id="navninfoDivP" data-type="navninfo" data-info="p">navninfop</div>
<div id="navninfoDivV" data-type="navninfo" data-info="V">navninfoV</div>

<div id="prisinfoDiv" data-type="prisinfo" data-info="">prisinfo</div>
<div id="prisinfoDivP" data-type="prisinfo" data-info="p">prisinfop</div>
<div id="prisinfoDivV" data-type="prisinfo" data-info="V">prisinfoV</div>

<div id="cableinfoDiv" data-type="cableinfo" data-info="">cableinfo</div>
<div id="cableinfoDivP" data-type="cableinfo" data-info="p">cableinfop</div>
<div id="cableinfoDivV" data-type="cableinfo" data-info="V">cableinfoV</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

$(function(){
    //Adding data to input
    //This will attach click event handler to all the divs 
    //whose id contains 'infoDiv'
    $('div[id*=infoDiv]').click(function() {
        $('#webhost_'+$(this).data('type')).data('myData', $(this).data('info'));
    });

    // Setting value to input on submit 
    $('#smt').click(function() {
        var $this;
        //This will loop through all the input fields whose id starts '
        //with 'webhost and prepend data('myData') to its value
        $("input[id^=webhost]").each(function(){
            $this = $(this);
            $this.val(($this.data("myData") || '') + $this.val());
        });
    });
});

I hope this helps you.

@Rails - Did you try this solution? – ShankarSangoli Aug 23 '11 at 03:00 — ShankarSangoli, Aug 23 '11 at 03:00

score 11 · Answer 2 · answered Aug 25 '11 at 03:09

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.

A nice step-through of the refactoring, +1. – Matt Sach Jan 08 '13 at 16:31 — Matt Sach, Jan 08 '13 at 16:31

score 2 · Answer 3 · answered Aug 20 '11 at 22:29

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.

score 0 · Answer 4 · answered Aug 16 '11 at 16:53

0

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.

answered Aug 16 '11 at 16:53

arunkumar

28,490
4
30
44

Is it not possiable to make a function that creates new data variables and associatate them on submit? – Rails beginner Aug 16 '11 at 17:20
Should be. You have some errors in your code though. With jquery if you know the id of the input element then you can select it like this `$("webhost_navinfo")` note you don't need # there. Also to set the value use `$("webhost_navinfo").value` the val function is different - http://api.jquery.com/val/ – arunkumar Aug 16 '11 at 17:30
@arunkumar: You’re wrong. You need to specify ID selectors with leading `#`. – Gumbo Aug 21 '11 at 11:12
@Gumbo - maybe i'm being blind, but don't all my ID selectors have a leading # ? Can you point out which line I'm missing it? – arunkumar Aug 21 '11 at 11:25
@arunkumar: It’s your previous comment: “With jquery if you know the id of the input element then you can select it like this `$("webhost_navinfo")` note you don't need `#` there.” – Gumbo Aug 21 '11 at 11:27
1

@Gumbo you are right, I don't know what I was smoking when I wrote that. – arunkumar Aug 21 '11 at 11:31

kapex · Answer 5 · 2011-08-17T09:15:16.813

0

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');

edited Aug 17 '11 at 09:15

answered Aug 16 '11 at 17:21

kapex

26,163
5
97
111

2

`const` in Javascript? Really? [It's only smatteringly supported.](http://stackoverflow.com/questions/130396/are-there-constants-in-javascript) – Lightness Races in Orbit Aug 16 '11 at 18:30
Didn't know it is unsupported in IE. I'll change it to `var`. – kapex Aug 17 '11 at 09:14
At least the first 2 lines make your code longer and just replace some repetitions by an equal number of new ones and an indirection. Not good really. – Jürgen Strobel Aug 24 '11 at 00:33
Repeating a variable over 50 with code completion support seems better than writing a string literal over 50 times. Not to mention if that string needs to be changed. The variable names aren't good though, if that is what you mean. They should semantically describe the content, not the content itself. – kapex Aug 24 '11 at 07:48

score 0 · Answer 6 · answered Aug 22 '11 at 06:25

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);
        }
    });
});

Mrchief · Answer 7 · 2011-08-25T03:17:35.300

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.

How to keep data functions and variables DRY?

7 Answers7

Linked