5
\$\begingroup\$

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:

<a href="a-link.html" data-intl-href="newval.html">test href</a>
<img src="https://test.com/logo.png" data-intl-src="newval.png">

Output HTML:

<a href="newval.html" data-intl-href="newval.html">test href</a>
<img src="newval.png" data-intl-src="newval.png">
\$\endgroup\$
5
\$\begingroup\$
  1. 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.

  2. 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?.

  3. 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.

  4. 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.

  5. Method Chaining

    replace() in filter() can be chained.

  6. 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]);
      }
    });
  }
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<a href="a-link.html" data-intl-href="newval.html" data-link="Like this???">test href</a>
<img src="https://test.com/logo.png" data-intl-src="newval.png" data-intl-alt="Can have multiple attributes">

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I wanted to check for data-intl in case there was another data tag present on that node. Your code fails if say for example there's a "data-link" attribute on the node \$\endgroup\$ – Seeker Dec 30 '16 at 5:11
  • \$\begingroup\$ I was trying to avoid using an each loop, thanks i just need to update the regex \$\endgroup\$ – Seeker Dec 30 '16 at 5:15
  • \$\begingroup\$ @Seeker I've added condition to check if the attribute starts with intl. \$\endgroup\$ – Tushar Dec 30 '16 at 5:19

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.