3
\$\begingroup\$

I wrote a solution for a codeCamp exercise:

Write a function called findShortestWordAmongMixedElements.

Given an array, findShortestWordAmongMixedElements returns the shortest string within the given array.

Notes:

  • If there are ties, it should return the first element to appear in the given array.
  • Expect the given array to have values other than strings.
  • If the given array is empty, it should return an empty string.
  • If the given array contains no strings, it should return an empty string.

Right now I essentially break this function into two parts. The filtering method—to just get the strings—and then reduce to find the shortest one among them.

function findShortestWordAmongMixedElements(arr) {
  var containsStrings = function(arr){
   return arr.every(function(cv){
      return Object.prototype.toString.call(cv) !== '[object String]';
    });
  }, shortestWord;

  if ((!(arr.length)) || ((containsStrings(arr)))) return '';

   arr = arr.filter(function(e, i, a){ 
     if (typeof e == 'string') {
       return e;
     }
   });

   shortestWord = arr.reduce(function(prev, next) {
     if (prev.length < next.length) {
         return prev;
     } else if (prev.length === next.length){
        return prev;
     } else {
        return next;
     }
   });

   return shortestWord;

}

My take is this is certainly readable which I like and I LOVE the helper function I made for the initial check which uses the every method.

Since (at first) I spent a lot of time trying to do all the operations within the body of the callback for the filter—I feel this take is like I used flat head screw driver for every screw instead of a phillips...

\$\endgroup\$
  • 1
    \$\begingroup\$ You don't need to put "update", "edit" or similar phrases into your question, there's a revision log. \$\endgroup\$ – Zeta May 11 '17 at 17:19
1
\$\begingroup\$

Probably the shortest way to do this is to use array.filter to only get the strings, array.sort to sort them by length, and get the first item of the resulting array.

function findShortestWordAmongMixedElements(arr){
  return arr.filter(e => typeof e === 'string').sort((a, b) => a.length - b.length)[0];
}

Now a few things with your code...

First, a mixed-type array is strange. I'm curious as to what the situation is why it's mixed. Anyways, it's best to keep arrays contain only one type to keep it predictable. Having it mixed makes receiving functions do unnecessary type checks, like containsStrings.

Also, just don't do type checks. If the function expects an array of strings, then assume it's an array of strings. If it contains something other than a string, let it blow up, let it throw an error. It's the consumer's responsibility to provide the correct input. Again, you'll end up doing unnecessary type checks that will only slow down the code.

containsStrings is also unnecessary. It's just a short-circuit if the array doesn't contain strings at all. filter can do that with one less call. If an array does not contain a string, filter will return an empty array, thus reduce won't run. If you give reduce an undefined as initial value, it will return undefined on an empty array.

\$\endgroup\$
  • \$\begingroup\$ The reason for the mixed-typed array (I updated my question) was due to the resource; Essentially it was asked by a coding school to get an idea of ones skill level. But what you said makes a lot of sense. Would you advise then to build into the function something to throw if someone did enter the wrong info? \$\endgroup\$ – Antonio Pavicevac-Ortiz May 11 '17 at 17:16
  • 6
    \$\begingroup\$ Although nice and succinct, sort is a very expensive way to find the shortest string. \$\endgroup\$ – Marc Rohloff May 11 '17 at 19:33
6
\$\begingroup\$

It seems odd to me that you loop through the array twice, once with every and once with filter. Why not just filter it and check if the filtered array has any elements? Also is there a reason why you used two different methods to check if the elements were strings? One more thing, I would have written the filter method as:

arr = arr.filter(function(e) { 
  return typeof e == 'string';
});

If you really want to do this using a single loop you could do something like:

    function findShortestWordAmongMixedElements(arr) {
      return arr.reduce( function(shortest, e) {
        return (typeof e == 'string') && (shortest=='' || e.length < shortest.length) ? e : shortest;
      }, '');
    };
    
    console.log( findShortestWordAmongMixedElements([
       1, 'longer', 2, 'short', 3, 'longest'
    ]) );

\$\endgroup\$
  • 2
    \$\begingroup\$ +1 for showing single loop solution. There is no need to iterate more than once. \$\endgroup\$ – Mike Brant May 11 '17 at 20:02
6
\$\begingroup\$

Remarks

  • You have few whitespace and parenthesis issues,
  • Use named function expressions (“named anonymous functions”) instead of anonymous functions, as it could potentially make debugging easier. It's nicely elaborated on in Juriy "kangax" Zaytsev's article,
  • The way you declared shortestWord is very unintuitive and not so readable, declaring it first would be much better;
  • Instead of iterating once to check if array contains strings and once to filter everything else out, you could just do the latter and check if it didn't resulted in an empty array;
  • Object.prototype.toString.call(cv) !== '[object String]' seem to me to be pretty overcomplicated, since you could just leverage typeof, which you actually did further in the code;
  • In arr.filter(function(e, i, a) i and a are unnecessary,
  • It's a good pattern to always use identity / strict equality operator (===) wherever possible,
  • In your reduce() you could get rid of entire else if if you changed < to <= in preceding if,
  • And most importantly: despite de jure both algorithms are \$O(n)\$, yours loops through the entire array three times, while it could do so only once.

Rewrite

It's definitely not the shortest solution provided in here, but it's performing only one iteration throughout the array and it's the most performant one, as well (see the next section). Note, that it uses elements of ES6, although it could be easily ported back to previous versions of JavaScript, e.g. using Babel transpiler.

const findShortestWordAmongMixedElements = array => {
  let shortest = '';
  
  for (let element of array) {
    if (typeof element === 'string' &&
        (shortest === '' || element.length < shortest.length)) {
      shortest = element;
    }
  }
  
  return shortest;
};

/* ===== */

console.log(
  findShortestWordAmongMixedElements([[], 0, null, undefined, 'aa', Infinity, true, 'a'])
);
/* Console formatting */
.as-console-wrapper { top: 0; }

Benchmark

Tested on Chrome 57 via jsbench.me.

  • Joseph's solution: 361,383 ops/s ±0.32%,
  • Marc's solution: 1,894,558 ops/s ±2.72%,
  • My solution: 13,532,763 ops/s ±0.28%.

Benchmark

\$\endgroup\$
  • 1
    \$\begingroup\$ While I understand what you mean by O(3n) and like your answer overall, I think is it worth pointing out that coefficients do not belong in Big-O. Both algorithms are O(n) even though one takes three times as long to run as the other. \$\endgroup\$ – gyre May 19 '17 at 2:29
  • \$\begingroup\$ @gyre: Yes, you are absolutely right. I just sometimes impulsively write that way, despite I'm fully aware that Big-O notation is meant to describe characteristics of growth and not the pace of it itself, and as such constants can be removed, because both 3n and n mean simply that it is linear. Anyway, I have edited my answer, thanks for comment ;). \$\endgroup\$ – Przemek May 20 '17 at 21:15
  • \$\begingroup\$ In your remarks you prefer named functions, yet in your code you write const func = array => { return expression } instead of the simpler function func(array) { return expression }. Why? \$\endgroup\$ – Roland Illig May 21 '17 at 6:25

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.