patternjavascriptMinor
Dynamic attribute key selector
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
The code works; I just want to know if I could have approached it differently.
Input HTML:
Output HTML:
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.