JavaScript while loop efficiency/performance

Question

I am currently working on a Codewars challenge, http://www.codewars.com/kata/your-order-please. The task is to take a string and re-order the words according to a value found in each word of the string, for example:"is2 Thi1s T4est 3a" the function should return "Thi1s is2 3a T4est".

I have written a solution that successfully passes all tests provided and returns the correct string. https://jsfiddle.net/louisstanard/5hygz6wb/

function order(words){
  var ordered = [];
  var arr = words.split(' ');
  var n = 1;
  while (n <= arr.length) {
    for (var i = 0; i < arr.length; i++) {
      var stringArr = arr[i].split('');
      stringArr.forEach(function(currentValue, index) {
        if (parseInt(currentValue) === n) {
          ordered.push(arr[i]);
          n++;
        }
      });
    }  
  }
  return ordered.join(' ');
}

My problem is that when I attempt to submit the solution I receive an error saying "Process was terminated. It took longer than 6000ms to complete". I chose to use a while loop because I want to continue iterating over each word in the string looking for a number until I've built up a new array whose length is the same as the original array.

I'm new to writing more performant JS, but I know that while loops (and probably a for nested inside of a while) can be very expensive operations. Does anyone have insight as to why this might be taking too long to run? Are there any glaring performance issues I'm not seeing? Or perhaps a better approach altogether? Thanks!

@thomas I forgot about the codereview community - you're right, that would be a better fit. Next time I'll post there. Thanks! — Charlie Stanard
– Charlie Stanard, Commented Oct 2, 2015 at 16:09

Shilly · Accepted Answer · 2015-10-02 15:46:13Z

1

Your code is running slow because you do a alot of unneeded calculations:

you dont cache the length of your two arrays, so every iteration, you check the length again.
you have 3 loops, where one loop would suffice.
you check every letter in the words, while parseInt(word, 10) already return the first number inside the word, so looping over the letters is useless.
your while loop and the first for loop basically do the same, since they both start at 0 and end at the amount of words the original string contains.
you push your results to an array instead of just setting ordered[i] = arr[i].
your for loops dont contain breaks, so you always continue the loop, even if you already found the correct result (the number in this case).
The logic of trying to build a new array until it has the length of the original array is flawed, since if you finish the loop, the new array will always have the length of the original, if your code is correct.

So in short, yes, it'll be way more performant to use one of the techniques the other ppl have posted, since that code is more optimized and doesn't contain redundant logic.

answered Oct 2, 2015 at 15:46

Shilly

8,5991 gold badge20 silver badges24 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Charlie Stanard Over a year ago

Thanks for the in-depth breakdown @Shilly! Much appreciated.

TbWill4321 · Accepted Answer · 2015-10-02 15:18:28Z

0

This is another way to do it:

function order(words){
  return words
    .split(' ')
    .sort(function( wordA, wordB ) {
      var numA = +(wordA.match(/\d+/g)[0]);
      var numB = +(wordB.match(/\d+/g)[0]);
      return numA - numB;
    })
    .join(' ');
}

answered Oct 2, 2015 at 15:18

TbWill4321

8,6863 gold badges30 silver badges25 bronze badges

2 Comments

charlietfl Over a year ago

providing another way to do it doesn't answer the question

TbWill4321 Over a year ago

OP also asked if there was: "perhaps a better approach altogether?". I gave one that was clear, concise, and doesn't loop for longer than 6000ms, which was his issue.

show-me-the-code · Accepted Answer · 2015-10-02 15:28:45Z

0

Take a look at an update to your Fiddle here. On my Chrome browser, shows an order of magnitude faster than the original.

function orderNew(words){
  var ordered = [];
  var arr = words.split(' ');  

    arr.forEach(function(item, index){
        var match = /\d+/.exec(item);
        ordered[parseInt(match[0])] = item;
    });
    return ordered.join(' ');
}

answered Oct 2, 2015 at 15:28

show-me-the-code

1,5531 gold badge13 silver badges14 bronze badges

Collectives™ on Stack Overflow

JavaScript while loop efficiency/performance

3 Answers 3

1 Comment

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related