0
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Brian May 2 '14 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.