10
\$\begingroup\$

I am new to programming. I have written a solution in two different ways, but would like to know what is considered a better solution, and why.

Additionally, in terms of performance, why would one be considered better?

Solution 1:

function titleCase(str) {
  str = str.toLowerCase();
  str = str.split("");
  str[0] = str[0].toUpperCase();
  for(i = 1; i<str.length; i++){
    if(str[i+1] == " "){
      str[i+2] = str[i+2].toUpperCase();
    }
  }
  str = str.join("");
  return str;
}

Solution 2:

function titleCase(str) {
  str = str.toLowerCase();
  str = str.split(" ");
  str = str.map(function(val){
    val = val.charAt(0).toUpperCase() + val.slice(1);
    return val;
  });
  str = str.join(" ");
  return str;
}
\$\endgroup\$
  • \$\begingroup\$ If I can add to what has been said, you can usually test performance by yourself! :) By benchmarking, you'll have the best hindsight on performance :p \$\endgroup\$ – IEatBagels Dec 10 '15 at 16:37
  • \$\begingroup\$ @TopinFrassi Very new to this. What would be a good way to benchmark this? Or, is there a good tutorial which teaches how to benchmark? \$\endgroup\$ – Frenchiejnr Dec 10 '15 at 16:38
  • \$\begingroup\$ I'm no benchmarking professional :p But I'm sure you can find something good with google. Or maybe someone will see this comment and propose a tool! :) \$\endgroup\$ – IEatBagels Dec 10 '15 at 16:39
  • 1
    \$\begingroup\$ The faster strategy depends on the browser. On Safari 8, the map()-based solutions (your Solution 2 and breckwagner's) are slower by 25%. On Firefox 42, your Solution 1 and Greg Burghardt's regex are slower by 25%. So, pick the solution you find the most elegant. \$\endgroup\$ – 200_success Dec 10 '15 at 23:10
  • 1
    \$\begingroup\$ @200_success wow. That is really interesting \$\endgroup\$ – Frenchiejnr Dec 10 '15 at 23:14
7
\$\begingroup\$

Given the two solutions, I think the second is stylistically a better way to do things in javascript for the reason that solution 1 is using the for loop which is not using the power of javascript as a functional programming language. An alternative modification of solution 2 would be make everything one statement and never change the value of str but instead return the value:

function titleCase(str) {
    return str.toLowerCase()
      .split(" ")
      .map(function(v){return v.charAt(0).toUpperCase() + v.slice(1)})
      .join(" ");
}

This modification means that the function will not mutate any variables. As for performance, the first solution runs slightly faster on my system using nodejs.

function time_it (fn, arg, n) {
    var time = Date.now()
    for(var i = 0; i < n; ++i) {
        fn(arg);
    }
    return Date.now() - time;
}

SENTANCE = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus quis augue consequat, ullamcorper ipsum in, scelerisque tortor. Nulla justo dolor, ultrices ac varius a, fringilla et nisi. Vestibulum tristique euismod turpis, sed fermentum nibh rutrum tempus. Fusce a metus tincidunt, convallis lectus sed, suscipit ipsum. Duis sagittis et dolor id dapibus. Morbi quam urna, tristique non bibendum sit amet, viverra eget magna. Duis felis nisi, sodales eu ante et, vulputate pellentesque sem. Integer luctus lacus blandit, euismod dui vel, ultrices nibh. Suspendisse potenti. Phasellus bibendum, quam sit amet vehicula dapibus, nunc augue blandit sapien, nec bibendum purus elit at turpis."

console.log("titleCase_1 (ms): " + time_it(titleCase_1,SENTANCE,100000))
console.log("titleCase_2 (ms): " + time_it(titleCase_2,SENTANCE,100000))
console.log("titleCase_3 (ms): " + time_it(titleCase_3,SENTANCE,100000))

The results

titleCase_1 (ms): 2688
titleCase_2 (ms): 4391
titleCase_3 (ms): 4364

This has to do with the line

val.charAt(0).toUpperCase() + val.slice(1);

and the fact that solution 1 doesn't make copies of the str in the loop like map does.

An argument in favor of the second solution is that its performance could be much better. Likely what is happening is the map function is running serially but in many functional languages the map function can or is parallelized automatically.

\$\endgroup\$
8
\$\begingroup\$

A regular expression is really all you need:

/(^| +)([a-zA-Z])/g

Then use the String#replace method and pass in a function:

function toTitleCase(s) {
    return s.toLowerCase().replace(toTitleCase.WORD_PATTERN, function(match, $1, $2) {
        return $1 + $2.toUpperCase();
    });
}

toTitleCase.WORD_PATTERN = /(^| +)([a-zA-Z])/g;

If you really feel like it, jsperf.com can give you a good idea what the performance is.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. I wasn't really looking for an alternative solution, but which of my 2 solutions are better, and what makes one preferable to the other. Thanks for the benchmarking link though. \$\endgroup\$ – Frenchiejnr Dec 10 '15 at 18:41
2
\$\begingroup\$

Version 2 may avoid all those reassignements by just chaining the methods:

return str.toLower.split(" ").map....;

This way you can read the function as a series of transormations, and the noisy repetition of str has gone away.

\$\endgroup\$
  • \$\begingroup\$ breckwagner has now posted a solution based on this idea. \$\endgroup\$ – 200_success Dec 10 '15 at 23:12
2
\$\begingroup\$

In addition to the other answers, your first version is incorrect. You offset the array index twice:

  • you loop from 1 to length
  • you work with index i+1

This will incorrectly work for "a sentence like this" because "sentence" will not get capitalized.

Let's fix it like

for(i = 1; i<str.length; i++){
    if(str[i] == " "){
        str[i+1] = str[i+1].toUpperCase();
    }
}

However, this is still not entirely correct. What if the last character in the string is a space? You then try to call toUpperCase() on an undefined element, which will crash.

I would always recommend version 2 (or preferably some of the alternatives posted in other answers) because they more clearly show what you are doing and thereby prevent mistakes in edge cases.

\$\endgroup\$
  • \$\begingroup\$ I think that the body of the loop should be if (str[i - 1] == ' ') { str[i] = str[i].toUpperCase(); } \$\endgroup\$ – 200_success Dec 11 '15 at 5:47
  • \$\begingroup\$ @200_success most likely, but still not very pretty :) my main point here is that those mistakes creep in because the code is so "code-ish" compared to the snippet by breckwagner, which is a straightforward way to write down exactly what you try to achieve \$\endgroup\$ – Vincent van der Weele Dec 11 '15 at 6:09

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.