# Is this node replacement code reasonable?

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.

• 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