Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;

    }
share|improve this question

2 Answers 2

up vote 5 down vote accepted

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();
}
share|improve this answer
    
Simplified a bit further by returning the last statement directly (one less line and assignment) and moved .toLowerCase() to operate on the final result. –  jfriend00 Mar 20 at 5:39

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.

share|improve this answer
    
Splitting functionality into 1 liners that you use only once usually fails code review. -1 –  konijn Mar 19 at 20:58
4  
@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. –  David Harkness Mar 19 at 21:40
    
Save possible upper-to-lower-to-upper conversion of the first character with str.substr(1).toLowerCase(). –  David Harkness Mar 19 at 21:43
1  
@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. –  200_success Mar 20 at 5:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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