Is this node replacement code reasonable?

Question

JSFiddle: http://jsfiddle.net/yMSHa/ (Note: Real code uses JQuery 1.4.1)

HTML:

<div data-name="action" data-id="1">Do It A 1</div>
<div data-name="action" data-id="2">Do It A 2</div>
<div data-name="action" data-id="3">Do It A 3</div>
<div data-name="action" data-id="4">Do It A 4</div>
<div data-name="action" data-id="5">Do It A 5</div>
<div style="display:none;">
 <div id="DoIt2">Do It B</div>
</div>

JS:

var RepDiv = $('#DoIt2');
var OldDiv = null;
$('div[data-name="action"]').each(function (i, f) {
 var actionid = $(f).attr('data-id');
 if (actionid) {
 $(f).css('cursor', 'pointer');
 var OnClick = function () {
 if (OldDiv) {
 RepDiv.replaceWith(OldDiv);
 OldDiv.click(OnClick);
 }
 OldDiv = $(this).replaceWith(RepDiv);
 };
 $(f).click(OnClick);
 }
});

Below is some simple JavaScript which replaces one node at a time when that node is clicked. The idea behind this code is that a user can click on a node for that node to be expanded with more options.

I'm looking for a general review. My main concern is whether this code is written properly in terms of good practices.

Answer 1

From a once over;

• i and f, even for forEach are not very good names, pick something more meaningful.

• if data-id is the action id, perhaps you could have gotten/set it with $().data('actionId') ?

• The way your replacement function uses OldDiv and RepDiv smells like global variable abuse

• Consider using smallCamelCase, actionid -> actionId, OldDiv -> oldDiv

• Consider caching $(f), since you access it several times ( var $f = $(f); and then only access $f )

• This : $(f).css('cursor', 'pointer'); really belongs in .css file

Other than that, your code looks okay to me.

• Brian: I resolved the global variable concerns by wrapping the code in a function (i.e., var GlobalWrapper = function() {/*My Code*/}();). Setting data-id with .data() is a bit more difficult than setting it in an attribute, as divs are generated by an asp.net repeater that looks something like <div data-id = '<%=Data.ID%>'>. The interesting thing about $(f).css('cursor', 'pointer'); is that it means the links don't look clickable until the events are loaded. I'm not sure if that should be considered a good thing.