5
\$\begingroup\$

I'm wondering if I am doing this in the most efficient way.

So I'm declaring my function el which is equal to a macro (Google Tag Manager), then I reassign or overwrite that previously-defined var which grabs the innerText/textContent of the element. It then converts the string to lowerCase and then capitalizes the first letter of the string, before finally returning the 'cleaned-up' element.

   function() {
      var el = {{element}}
      el = (el.innerText || el.textContent);
      el = el.toLowerCase();
      el = el.charAt(0).toUpperCase() + el.slice(1);

      return el;

    }
\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

You can using chaining, move the toLowerCase() to simplify a little and return the last statement directly:

function() {
      var el = {{element}};
      el = (el.innerText || el.textContent);
      return el.charAt(0).toUpperCase() + el.slice(1).toLowerCase();
}

I'm personally not a fan of using a single variable for many purposes (I think it interferes with readability and maintainability) so I would probably do this:

function() {
      var el = {{element}};
      var text = (el.innerText || el.textContent);
      return text.charAt(0).toUpperCase() + text.slice(1).toLowerCase();
}
\$\endgroup\$
1
  • \$\begingroup\$ Simplified a bit further by returning the last statement directly (one less line and assignment) and moved .toLowerCase() to operate on the final result. \$\endgroup\$
    – jfriend00
    Mar 20, 2014 at 5:39
5
\$\begingroup\$

I think you have a valid concern about the repeated reassignment.

Based on the Single-Responsibility Principle, I would split this function into two: one that interfaces with the DOM, and another that transforms a string.

function content(element) {
    var el = {{ element }};
    return el.innerText || el.textContent;
}

function titleCase(str) {
    return str.charAt(0).toUpperCase() + str.substr(1).toLowerCase();
}

// The original function in question
function() {
    return titleCase(content(element));
}

Decomposing the problem that way addresses the code smell at the root cause.

\$\endgroup\$
5
  • \$\begingroup\$ Splitting functionality into 1 liners that you use only once usually fails code review. -1 \$\endgroup\$
    – konijn
    Mar 19, 2014 at 20:58
  • 4
    \$\begingroup\$ @konijn Big assumption with "you'll only use once". titleCase and content look like they could be reused a lot assuming this application has more than one DOM element. Besides, these methods are now testable and composable which can be a big win. \$\endgroup\$ Mar 19, 2014 at 21:40
  • \$\begingroup\$ Save possible upper-to-lower-to-upper conversion of the first character with str.substr(1).toLowerCase(). \$\endgroup\$ Mar 19, 2014 at 21:43
  • 1
    \$\begingroup\$ @DavidHarkness The order of operations will make a negligible difference in performance, but I think that the parallelism of putting toUpperCase()/toLowerCase() last is appealing. I've incorporated the change into Rev 2. \$\endgroup\$ Mar 20, 2014 at 5:42
  • \$\begingroup\$ content isn't a verb, functions do stuff - what does it do? getConetentFromElement or just getContent? \$\endgroup\$
    – Julix
    Jan 4, 2023 at 19:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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