21
\$\begingroup\$

The challenge is simple:

Return the length of the longest word in the provided sentence.

The solution is just as simple:

function findLongestWord(str) {
  arr = str.split(' ');
  size = 0;
  for (var s in arr) {
    if (arr[s].length > size) {
      size = arr[s].length;
    }
  }
  return size;
}

However, I vaguely remember you're not supposed to use for..in in JavaScript unless absolutely necessary. What would be the more idiomatic approach for this loop?

\$\endgroup\$
9
  • \$\begingroup\$ for(let i=0; i<arr.length(); ++i) { let s = arr[i]; if(s.length>size) ... } ? \$\endgroup\$ – Charlie Dec 22 '15 at 23:42
  • \$\begingroup\$ Maybe even arr.sort(s=>s.length)[0].length; (sort by length of string, get shortest string, get length) or arr.map(s=>s.length).sort()[0]; (make new array with lengths of strings in arr, sort that array, get shortest size) \$\endgroup\$ – Charlie Dec 22 '15 at 23:48
  • 2
    \$\begingroup\$ Are you using ES6? \$\endgroup\$ – SirPython Dec 22 '15 at 23:54
  • 2
    \$\begingroup\$ Definitely write in ES6 and transpile using Babel. The productivity gain is just too valuable. \$\endgroup\$ – Dan Dec 23 '15 at 9:36
  • 1
    \$\begingroup\$ @NickUdell Punctuation is considered part of a word in the challenge. \$\endgroup\$ – Mast Dec 23 '15 at 11:49
21
\$\begingroup\$

First, I'd not name the function findLongestWord as you're not looking for the longest word, but the length of the longest word. Try getLongestWordLength instead.

You're forgetting var for your variables. This makes them shoot up to the global scope and be declared there, and we don't want that to happen. For ES6, there's also let.

for-in is only advisable on objects, and even on objects you guard it with hasOwnProperty. That's because it iterates through prototype properties (things other than the array elements or instance properties). A regular loop (for or while) while incrementing an index until length would be better. But there's an even better approach...

You can create a map of lengths by using map on your split string, returning the length of the strings. Then you use Math.max to get the largest number in the array of lengths. We can use the spread operator (...) to spread the array as arguments to Math.max.

function getLongestWordLength(str){
  return Math.max(...(str.split(' ').map(s => s.length)));
}

The above is ES6 syntax. The ES5 equivalent would look like the following. One notable difference, aside from the more verbose map is the use of apply to provide Math.max with a dynamic set of arguments.

function getLongestWordLength(str) {
    return Math.max.apply(Math, str.split(' ').map(function (s) {
        return s.length;
    }));
}

document.write(getLongestWordLength('The quick brown fox jumps over the lazy doge'));

\$\endgroup\$
1
  • \$\begingroup\$ This looks like the idiomatic approach, yes. It makes sense not to iterate over information we're not interested in anyway. Great answer! \$\endgroup\$ – Mast Dec 23 '15 at 0:31
7
\$\begingroup\$

Update/Edit now that I'm not on just my phone anymore, I'm adding some more context and "why/how Array.prototype.reduce can be useful.

Original answer (shortest)

someString.split(' ')
  .reduce((max, word) => word.length > max ? word.length : max, 0);

Short version/solution (stripped of comments/etc...)

// Make use of `map` and `reduce`
const longestWordLength = str => str.split(' ')
  .map(w => w.replace(/\W/, ''))
  .reduce((longest, current) => (current.length > longest.length) ? current : longest)
  .length;

longestWordLength("Mary had a little lamb.");
// => 6

Of course you can solve the same problems 100 ways (both the beauty and curse of programming), but I've found that putting some time into learning reduce and other data transformation, functional approaches has paid dividends in code quality and happiness :)

If map/reduce/ES6 is new to you, I highly recommend going and checking out these short, free videos from Egghead.io on the topic:

This JSBin is also public and is up and running with some related code/examples (ES6 enabled via Babel) if you want to play with some of the code


Longer version/solution and some of the "why". Please refer to comments/documentation.

Note, that I am accounting for punctuation and not counting it as part of the word.

/**
 * Returns the longest word's length
 *   Doesn't care if findLongestWord is using reduce, sort, etc... internally.
 *   Either way, we have to find the longest word. Then we just ask for its length
 * @param  {string} str The string to scan
 * @return {integer}    The length of the longest word found
 */
function findLongestWordLength(str) {
  return findLongestWord(str).length;
}

/**
 * Find the longest word present in a given string
 * @param  {string} str The string to scan
 * @return {string}     The longest word found
 */
function findLongestWord(str) {
  return str.split(' ')
    .map(cleanWord)
      /*
      Reduce this array of words down to the longest word in the collection
       Is the current word we are looking at longer than our longest?
       If it is, set it to be the longest.
       Next!
       */
    .reduce(reduceToLongest);
}

/**
 * Strip down each item resulting from split(' ') to a measurable "word" (i.e. strip non [a-Z] characters)
 * This could be as simple or complex as it needs to be
 * @param  {string} word Example: "lastword."
 * @return {string}      Example: "lastword"
 */
function cleanWord(word) {
  return word.replace(/\W/, '');
}

/**
 * Reducer for "finding the longest string"
 * @param  {string} longest The currently "longest" string
 * @param  {string} current The string being examined
 * @return {string}         The larger of longest vs. current
 */
function reduceToLongest(longest, current) {
  return (current.length > longest.length)
    ? current
    : longest;
}
\$\endgroup\$
2
  • \$\begingroup\$ I assume max here is where I used size? Can you explain why your alternative is better/simpler/etc. than what I used? \$\endgroup\$ – Mast Dec 23 '15 at 8:25
  • \$\begingroup\$ Yes, max was representing "the largest length/number we've come across so far". I've added updated and more complete code examples along with some links to learning resources relating to "why functional/.map/.forEach can be beneficial over traditional for loops". I hope you find it useful! \$\endgroup\$ – ErikTheDeveloper Dec 23 '15 at 19:50
6
\$\begingroup\$

It's not that you shouldn't use for/in unless absolutely necessary; it's that you shouldn't use it for arrays.

It should only be used for objects. For this code, you should be using a normal for loop.


I understand that this violates the programming challenge requirements, but this is just as a future tip.

I think it would be better practice to accept the array of strings rather than a string with the words separated by a space in it. Your code will be a lot more versatile if the caller can determines what is and what isn't a word themselves, and can use your function however needed for their data set.

For example, if the caller has a string like this:

"foo:bo:hello"

(however they got that does not matter)

Your function would not work in this case. However, if you only accept an array, then the caller can do whatever they want to this string to determine what "words" are.


You're creating two global variables in your function: arr and size.

Don't. They are bad. Use var.

\$\endgroup\$
4
  • \$\begingroup\$ The code isn't accepting arrays, it's accepting strings. It's split in arrays of strings later. If it doesn't work for foo:bo:hello that's too bad, caller should've used spaces instead of :. The function doesn't have to be smart, as long as it's straightforward. It's an easy task after all. Wouldn't implementing your answer result in over-engineering? Good catch on the var though. And a normal for loop could be faster, I have no idea. That's why I asked. \$\endgroup\$ – Mast Dec 23 '15 at 0:09
  • \$\begingroup\$ I know it's not accepting an array of strings; I said it should. What if the caller needs a function just like the one you are providing but instead they have a different delimiter? I'm not trying to make it smart or less straightforward; how is having the function accept an array of strings over-engineering it? I'm simply saying that your function is being overly "specific". \$\endgroup\$ – SirPython Dec 23 '15 at 0:11
  • \$\begingroup\$ Note the programming-challenge. A sentence is provided, not an array. I'd need a function to automatically figure out what the delimiter is since I can't give it to it. That sounds not too straight-forward, unless you know something which makes that very easy. \$\endgroup\$ – Mast Dec 23 '15 at 0:18
  • \$\begingroup\$ @Mast Right, forgot about the challenge. In this case, if you took the change, calling it would still remain straightforward: getLongestWord(str.split(' ')) \$\endgroup\$ – SirPython Dec 23 '15 at 0:24
2
\$\begingroup\$

As I also said in the comments, I would approach this by letting JavaScript sort the array.

However, if you don't want to return the size, but the word itself (as the function name suggests), you could use something like

function findLongestWord(str) {
  let arr = str.split(' ');
  let sortbylength = arr.sort(s=>s.length);
  return sortbylength[0];
}

This sorts the strings in arr by their length, then returns the first item (the smallest one).

Okay, so 1) The function name is misleading. I would choose something more like findSizeOfLongestWord. 2) Then I'd use this code:

 function findLongestWord(str) {
   let arr = str.split(' ');
   let sortbylength = arr.sort(s=>s.length);
   return sortbylength[0].length;
 }

It first sorts the parts of the string by their length, gets the first item (shortest), gets its length, and returns that.

I like this because it uses less code, and is more readable imho.

As SirPython has already said, you shouldn't use a fixed delimiter if this is part of an API or something similar, but get the delimiter from an argument.


Warning: This function uses ECMAScript! It's implemented in most modern browsers, but maybe you don't want it in this case.

\$\endgroup\$
8
  • \$\begingroup\$ The OP does not specify that they are using ES6. \$\endgroup\$ – SirPython Dec 22 '15 at 23:54
  • \$\begingroup\$ Also, I'm not sure if they answer is more efficient. In the OP's code, they are splitting the string once and then iterating through it once. Here, you are splitting the string once but iterating through it twice with map and sort. \$\endgroup\$ – SirPython Dec 22 '15 at 23:56
  • \$\begingroup\$ @SirPython The second function only iterates through them once. Use .length after the return statement to return just the length. \$\endgroup\$ – Charlie Dec 22 '15 at 23:57
  • \$\begingroup\$ I don't think that second snippet works as you think. When I try it, I simply get the string back. \$\endgroup\$ – SirPython Dec 23 '15 at 0:01
  • 2
    \$\begingroup\$ Sorting would be O(n log n), whereas it should be possible to find the maximum in O(n). \$\endgroup\$ – 200_success Dec 23 '15 at 8:42
2
\$\begingroup\$

Your code is good, but it could be improved:

  • findLongestWord: You actually just find the length, I would title it findLongestWordLength instead, or make it return the word.
  • As SirPython points out, you should add the ability to use a custom delimiter.
  • If you attached this function to the String prototype, you'd find that this function would look a lot simpler:

String.prototype.findLongestWordLength = function(){
    //...
}
"Lorem Ipsum...".findLongestWordLength();

Inside prototype chains attached to primitive types, you can access the parent object, which in this case would be "Lorem Ipsum..." with this, however, this only has a getter property, you cannot reassign this, however, you can assign properties to this.

  • You don't need to use for ... in, I would avoid using it. Personally, I usually use forEach and normal for loops only.

  • However, like Charlie says, you can simply sort the array.

String.prototype.findLongestWord = function(delimiter){
    delimiter = delimiter || " ";
    return this.split(delimiter).sort(function(a, b){
        return b.length - a.length;
    })[0];
}

This example would return the word, whereas if you added .length after [0], it would return the length.


Adding to String.prototype is one method, however as @SirPython, @JosephTheDreamer and @DanPantry point out in the comments, you shouldn't add to prototypes of vendor types unless you really know what you're doing, as it makes your code really hard to maintain in terms of backwards and forward compatibility.

It's probably better to add the string as a parameter. (Make sure it's passed in first, JavaScript has optional parameters)

Additionally, in this case, you should throw an error if it's not passed in the function.

function findLongestWord(string, delimiter){
   if (!string){ throw new Error("String needed"); }
   delimiter = delimiter || " ";
   return this.split(delimiter).sort(function(a, b){
       return b.length - a.length;
   })[0];
}
\$\endgroup\$
8
  • 4
    \$\begingroup\$ I believe it is bad practice in JavaScript to modify an existing "native" prototype (or so I've read). \$\endgroup\$ – SirPython Dec 23 '15 at 0:26
  • \$\begingroup\$ It's not an existing prototype, it's extending a primitive prototype. I've never come across anything saying this is bad practice. \$\endgroup\$ – Quill Dec 23 '15 at 0:28
  • \$\begingroup\$ @Quill-HATMANIAC en.wikipedia.org/wiki/Prototype_JavaScript_Framework#Problems . Name collisions against future APIs would be the most common problem. \$\endgroup\$ – Joseph Dec 23 '15 at 0:39
  • \$\begingroup\$ Uh, @JosephtheDreamer that's a link to a library named Prototype.JS... \$\endgroup\$ – Quill Dec 23 '15 at 0:41
  • 1
    \$\begingroup\$ +1 @SirPython, it's against best practices to modify the prototype of a vendor type due to backwards/forwards compatibility issues. Also it would be very strange for an external user of your library to come in and see this weird unknown method on a string and have to go and find out what it is. Just use a scope-bound function (or one imported from another module). \$\endgroup\$ – Dan Dec 23 '15 at 9:22
1
\$\begingroup\$

Inspired by the interesting solutions from @JosephtheDreamer and ErikTheDevelopper, I was curious to find a solution that'd:

  1. Keep as concise as possible
  2. But include the improvement about not considering punctuation
  3. And avoid to multiply the internal use of a lot of successive temp arrays

So it actually comes like this (ES6 version):

function getLongestWordLength(str){
  return Math.max(...str.match(/(\w+)/g).map(w => w.length));
}
\$\endgroup\$

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.