HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Dynamic attribute key selector

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
selectorkeyattributedynamic

Problem

I'm basically trying to build a function that searches all nodes on the DOM to check for data attributes, then swap them out with whatever is stored with the "data-intl-" attribute. For example, if someone had data-intl-attr, whatever attr is on, that node would get replaced by what's in data-intl-attr.

The code works; I just want to know if I could have approached it differently.

var all = $('*');

$.map(all,function(el) {

    filter($(el).data(), function(clean){
      var originalAttrVal = $(el).attr(clean);
      var intlAttr = $(el).attr("data-intl-" + clean);

      $(el).attr(clean, intlAttr);

    });

});

function filter(foo, cb){
        var capture;
    for(var i in foo) {
      if (foo.hasOwnProperty(i)) {
        i = i.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
        i = i.replace(/intl-/g, '');
        capture= i;
     }      
     cb(capture);
  }

}


Input HTML:

test href


Output HTML:

test href

Solution


  • Naming Conventions



The variable all doesn't tell what it contains. allElements does reflect what it contains.

clean, i, foo, capture - huh?

filter, the name of the function has nothing to do with filtering. It is iterating over some data and calling provided callback for each property.

  • Global Variables



all is global variable. Global variables is bad practice. Please see Why are global variables considered bad practice? and I've Heard Global Variables Are Bad, What Alternative Solution Should I Use?.

  • Using Proper Methods



map is used to update each element in collection. Here, map is used just to iterate over the elements. You can use each, for, forEach, etc. in such case.

  • Diving into DOM



In filter callback, you're diving into DOM three times to get the same element. The element can be cached and reused.

  • Method Chaining



replace() in filter() can be chained.

  • Unnecessary code



foo.hasOwnProperty(i) is not required as .data() only returns data-* properties in an object.
.toLowerCase() can be safely removed and i flag on the next replace regex can be used.

Also, HTML is case insensitive. No transformation is needed when changing attribute.

That is too much code for such a small functionality. I've rewritten the code. See the explanation in the code comments.



// Select all elements in DOM
// and iterate over them
$('*').each(function() {
// Cache the element reference
// $(this) refers to the current element in the collection
var $this = $(this);

// Get the key-value of all
data-* attributes
// on current element
var data = $this.data();

// Get all the keys of data-attributes
var keys = Object.keys(data);

// Check if this element have data-* attribute on it
if (keys.length) {

// Iterate over each data attribute
keys.forEach(function(key) {

// Check if attribute starts with
intl followed by Uppercase character
if (/^intl[A-Z]/.test(key)) {
// Get the attribute name
// As the attribute name is camelCase, last word is the attribute name
var attribute = key.match(/[A-Z][a-z]*$/)[0];

// Update the attribute in DOM
$this.attr(attribute, data[key]);
}
});
}
});


test href

Context

StackExchange Code Review Q#151215, answer score: 5

Revisions (0)

No revisions yet.