current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I'm trying to figure out what is best and fastest implementation of a browser-like find and highlight function inside a website using JavaScript. This function is for one HTML element without any children. Of course it can be expanded to elements with children with some simple loops. That is not part of question. I wrote this small code to make something working here:

CSS

span.highlight{background:yellow; padding:3px;}

HTML

<input type="search"/>
<p>The Ama...</p>

JavaScript

var s = document.querySelector('input[type="search"]'),
    p = document.querySelector('p'),
    find = function(){
    var words = p.innerText.split(' ');
    for(var i=0; i<words.length; i++){
        if(words[i].toLowerCase() == s.value.toLowerCase()){
            words[i] = '<span class="highlight">' + words[i] + "</span>";
            p.innerHTML = words.join(' ');
        }
        else{

        }   
    }
}
s.addEventListener('keydown', find , false);
s.addEventListener('keyup', find , false);

This code is working fine. Checkout live example here. But this is not fast and sometimes crash the browser. How can I make this functionality with a better approach?

Please note I don't want a full code review here. So don't remind me that querySelector is not supported in IE7 and same things.

share|improve this question
    
Is there a point in getting and lowercasing words on each call to find()? Why not just do that once on onload? My javascript-fu is pretty low, so even if it's something obvious please explain... –  Yannis Nov 28 '11 at 5:39
    
Because I don't wanna mess words array with converting it to lower case. I'm keeping element's actual text in that array –  Mohsen Nov 28 '11 at 6:12
    
Ah yes you need the actual text to highlight it. Still I think it would be significantly faster if you've built the array once and outside of the loop, you could keep two values for each word (the lowercase to compare and the original to highlight). –  Yannis Nov 28 '11 at 6:19
1  
It occurs to me you can use Array.reduce here. Something like p.innerHTML = p.innerText.split(' ').reduce(function (prev, cur) {return prev + ' ' + ((cur.toLowerCase() === s.value.toLowerCase()) ? '<span class="highlight">' + cur + "</span>" : c;}, ""). While so many browsers have yet to implement a fast reduce it's not the best approach, so I'm not giving it as an answer, but food for thought. –  kojiro Nov 29 '11 at 0:16

1 Answer 1

up vote 1 down vote

Let's start by optimising your code. Simply said, loops are the danger here.

  1. A while loop can be quicker than a for loop, especially in cases like this. Get the length once.
  2. Avoid looking into arrays. Do word = words[i] once, which is much faster.
  3. You are joining in every iteration. You should join once only after you have updated your array.
  4. I guess the empty else isn't done yet.

var s = document.querySelector('input[type="search"]'),
    p = document.querySelector('p'),
    find = function(){
        var words = p.innerText.split(' '),
            i = words.length,
            word = '';

        while(--i) {
            word = words[i];
            if(word.toLowerCase() == s.value.toLowerCase()){
                words[i] = '<span class="highlight">' + word + "</span>";
            }
            else{

            }   
        }

        p.innerHTML = words.join(' ');
    }

s.addEventListener('keydown', find , false);
s.addEventListener('keyup', find , false);

Last but surely not least, the page you want to read.

share|improve this answer
    
Wow! Really faster: jsbin.com/ebucaz/3/edit I didn't accept as answer to see others answers. But this is great and will accept if nobody else replied –  Mohsen Nov 28 '11 at 18:30
    
I'm curious if it would be faster to use a stack. while (words) { word = words.pop(); …} –  kojiro Nov 28 '11 at 23:39
    
There's really no need for another function call. You want to avoid those. Plus popping out the value doesn't benefit you. It's cute, but not faster I'm afraid :) –  lcampanis Nov 29 '11 at 9:43
    
Mohsen - Would you mind accepting the answer if this helped you? Thanks! –  lcampanis Dec 13 '11 at 11:51

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.