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.