8
\$\begingroup\$

This is the code I wrote for making a JavaScript typeahead/autocomplete, which displays cities based on the user input.

It would be great if you could suggest some improvements on the code quality.

'use strict';

var data = ['alabama','alaska', 'arizona','arkansas','california', 'colorado', 'connecticut', 'delaware'];

/** 
 * variables: searchBox is where you type the city name
*/
var searchBox = getElementById('typeahead');


/**
 * methods
 */
function getElementById(id) {
    return document.getElementById(id);
}

function iterate(arr, callback) {
    for(var i=0;i<arr.length;i++) {
        callback(arr[i], i);
    }
}

var search = function(val) {
    var arr = [];
    iterate(data, function(item, index) {
        if(item.indexOf(val) !== -1) {
            arr.push(item);
        }
    })
    return arr;
}

var addEvent = function(elm) {
    elm.addEventListener('click', function(event) {
        if(event.target !== event.currentTarget) {
            searchBox.value = event.target.textContent; 
            removeChild();    
        }
        event.stopPropagation();
    });
}

var appendChild = function(parent, child) {
    parent.appendChild(child);
}

var removeChild = function() {
    var ul = getElementById('searchResults');
    if(ul) {
        document.body.removeChild(ul);
    }
}

var bindData = function(arr) {
    
    var ul = document.createElement('ul');
    ul.setAttribute('id','searchResults')
    iterate(arr, function(item, index) {
        var li = document.createElement('li');
        var a = document.createElement('a');
        a.textContent = item;
        appendChild(li, a)
        appendChild(ul, li);
    });

    appendChild(document.body, ul);
    addEvent(ul);
}

function debounce(func, wait, immediate) {
    var timeout;
    return function() {
        var context = this, args = arguments;
        var later = function() {
            timeout = null;
            if(!immediate) {
                func.apply(context, args);
            }
        };
        var callNow = immediate && !timeout;
        clearTimeout(timeout);
        timeout = setTimeout(later, wait);
        if(callNow) {
            func.apply(context, args);
        }
    };
}

var typeAhead = debounce(function(event) {
    removeChild();
    var value = event.target.value;
    if(value !== '') {
        var arr = search(value);
        bindData(arr);
    }
}, 400);

searchBox.addEventListener('keyup', typeAhead);
<input type="text" placeholder="type to search" id="typeahead">

\$\endgroup\$
7
  • \$\begingroup\$ iterate() is essentially the standard JS function Array#forEach; are you deliberately reimplementing it? \$\endgroup\$ – BenC Sep 12 '17 at 7:08
  • \$\begingroup\$ @BenC yes I have deliberately implemented it. I know I could have used forEach here which would have been a better approach than making a new function which does the same thing. \$\endgroup\$ – Rahul Arora Sep 12 '17 at 12:57
  • \$\begingroup\$ @BenC It will be great if you can tell me more improvements and comment on the overall code quality. \$\endgroup\$ – Rahul Arora Sep 12 '17 at 12:57
  • \$\begingroup\$ @RahulArora just so you know, the people on Stack Overflow were wrong, this question is not off-topic there, nor is it disallowed to post the same question across sites on the network. \$\endgroup\$ – Patrick Roberts Sep 13 '17 at 23:23
  • \$\begingroup\$ @PatrickRoberts: My question got downvoted, hence less people bother to open it or answer it. Do you suggest posting it again? \$\endgroup\$ – Rahul Arora Sep 13 '17 at 23:25
3
\$\begingroup\$

DOM querying is expensive

Search on Google for "js DOM query expensive" and you will likely find many posts from the past 10 years that discuss how in-efficient it is to be querying the DOM each time. Stop Writing Slow Javascript appears to be somewhat recent - see the section Cache DOM Lookups. This answer on SO about various DOM-selector functions will likely be interesting as well.

In the code below, notice that the variables searchBox and ul are declared at the top

var searchBox, ul;

Those variables don't get assigned until the callback for a new event listener for the DOMContentLoaded event is triggered.

document.addEventListener("DOMContentLoaded", function(event) {
    searchBox = getElementById('typeahead');
    searchBox.addEventListener('keyup', typeAhead);

    ul = document.createElement('ul');
    ul.setAttribute('id', 'searchResults')
    appendChild(document.body, ul);
    hideList();
    addEvent(ul);
});

You will also notice that in that event callback, the unordered list (i.e. ul) is added to the document and hidden. That way there is only one list ever added, and because of that, we can hide and show the list instead of removing it. The function removeChild can be replaced with a function hideList that will set the display style (using HTMLElement.style) to none. Additionally, the call to addEvent() was moved into that callback function, so we only add the event listener once. One could also use event delegation and have one click handler for the whole page - it would just handle clicks differently depending on the type of element clicked.

Keep the scopes limited

Another thing that article mentions is limiting the scope as much as possible (see the section Keep your scopes close and your scope even closer). One way to not clutter up the global namespace is to wrap the code with an IIFE:

;(function(window, document, undefined) {
    var data = ['alabama', 'alaska', 'arizona', 'arkansas', 'california', 'colorado', 'connecticut', 'delaware'];
    //...
})(window, document);

See the changes applied below. There are likely other improvements that can be made as well...

'use strict';;
;(function(window, document, undefined) {
  var data = ['alabama', 'alaska', 'arizona', 'arkansas', 'california', 'colorado', 'connecticut', 'delaware'];


  /** 
   * variables: searchBox is where you type the city name, ul is the list of suggestions
   */
  var searchBox, ul;

  /**
   * methods
   */
  function getElementById(id) {
    return document.getElementById(id);
  }

  function iterate(arr, callback) {
    for (var i = 0; i < arr.length; i++) {
      callback(arr[i], i);
    }
  }

  var search = function(val) {
    var arr = [];
    iterate(data, function(item, index) {
      if (item.indexOf(val) !== -1) {
        arr.push(item);
      }
    })
    return arr;
  }

  var addEvent = function(elm) {
    elm.addEventListener('click', function(event) {
    console.log('list click - target: ',event.target,' currentTarget: ',event.currentTarget);
      if (event.target !== event.currentTarget) {
        searchBox.value = event.target.textContent;
        hideList();
      }
      event.stopPropagation();
    });
  }

  var appendChild = function(parent, child) {
    parent.appendChild(child);
  }
  var hideList = function() {
    ul.style.display = 'none';
    while (ul.firstChild) {
      ul.removeChild(ul.firstChild);
    }
  }

  var bindData = function(arr) {
    ul.style.display = '';
    iterate(arr, function(item, index) {
      var li = document.createElement('li');
      var a = document.createElement('a');
      a.textContent = item;
      appendChild(li, a)
      appendChild(ul, li);
    });
  }

  function debounce(func, wait, immediate) {
    var timeout;
    return function() {
      var context = this,
        args = arguments;
      var later = function() {
        timeout = null;
        if (!immediate) {
          func.apply(context, args);
        }
      };
      var callNow = immediate && !timeout;
      clearTimeout(timeout);
      timeout = setTimeout(later, wait);
      if (callNow) {
        func.apply(context, args);
      }
    };
  }

  var typeAhead = debounce(function(event) {
    hideList(); //removeChild();
    var value = event.target.value;
    if (value !== '') {
      var arr = search(value);
      bindData(arr);
    }
  }, 400);

  document.addEventListener("DOMContentLoaded", function(event) {
    searchBox = getElementById('typeahead');
    searchBox.addEventListener('keyup', typeAhead);

    ul = document.createElement('ul');
    ul.setAttribute('id', 'searchResults')
    appendChild(document.body, ul);
    hideList();
    addEvent(ul);
  });
})(window, document);
<input type="text" placeholder="type to search" id="typeahead">

\$\endgroup\$
2
  • \$\begingroup\$ Do I also remove the event listeners once list is no longer in use to improve performance? \$\endgroup\$ – Rahul Arora Sep 13 '17 at 22:27
  • \$\begingroup\$ oh yes - actually the call to addEvent() could be moved into the callback for the DOMContentLoaded event, after the unordered list (i.e. ul) is created... If there end up being multiple lists/typeahead controls on the page, I would recommend using event delegation so that can be achieved with one event listener instead of one for each element. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 15 '17 at 16:29
1
\$\begingroup\$

Well it has been a year since I supplied an answer to this question but bearing in mind that you commented "@BenC yes I have deliberately implemented it. I know I could have used forEach here which would have been a better approach than making a new function which does the same thing." the functions iterate() and getElementById() can be be removed.

Instead of using iterate(), use Array functions like Array.prototype.forEach() and Array.prototype.filter(). Because filter() runs a function for every item in the array, that is basically reinventing the wheel for many of those functional array methods.

Take for example the function search():

var search = function(val) {
    var arr = [];
    iterate(data, function(item, index) {
        if(item.indexOf(val) !== -1) {
            arr.push(item);
        }
    })
    return arr;
}

Instead use the built-in array method filter():

var search = function(val) {
    return data.filter(function(item, index) {
        return item.indexOf(val) !== -1;
    });
}

And similarly, bindData() can use arr.forEach() instead of iterate().

And getElementById() doesn't really wrap anything special - it just calls document.getElementById() so there isn't really any advantage to it.

'use strict';;
;(function(window, document, undefined) {
  var data = ['alabama', 'alaska', 'arizona', 'arkansas', 'california', 'colorado', 'connecticut', 'delaware'];


  /** 
   * variables: searchBox is where you type the city name, ul is the list of suggestions
   */
  var searchBox, ul;

  /**
   * methods
   */

  var search = function(val) {
    return data.filter(function(item, index) {
      return item.indexOf(val) !== -1;
    });
  }

  var addEvent = function(elm) {
    elm.addEventListener('click', function(event) {
      if (event.target !== event.currentTarget) {
        searchBox.value = event.target.textContent;
        hideList();
      }
      event.stopPropagation();
    });
  }

  var appendChild = function(parent, child) {
    parent.appendChild(child);
  }
  var hideList = function() {
    ul.style.display = 'none';
    while (ul.firstChild) {
      ul.removeChild(ul.firstChild);
    }
  }

  var bindData = function(arr) {
    ul.style.display = '';
    arr.forEach( function(item, index) {
      var li = document.createElement('li');
      var a = document.createElement('a');
      a.textContent = item;
      appendChild(li, a)
      appendChild(ul, li);
    });
  }

  function debounce(func, wait, immediate) {
    var timeout;
    return function() {
      var context = this,
        args = arguments;
      var later = function() {
        timeout = null;
        if (!immediate) {
          func.apply(context, args);
        }
      };
      var callNow = immediate && !timeout;
      clearTimeout(timeout);
      timeout = setTimeout(later, wait);
      if (callNow) {
        func.apply(context, args);
      }
    };
  }

  var typeAhead = debounce(function(event) {
    hideList(); //removeChild();
    var value = event.target.value;
    if (value !== '') {
      var arr = search(value);
      bindData(arr);
    }
  }, 400);

  document.addEventListener("DOMContentLoaded", function(event) {
    searchBox = document.getElementById('typeahead');
    searchBox.addEventListener('keyup', typeAhead);

    ul = document.createElement('ul');
    ul.setAttribute('id', 'searchResults')
    appendChild(document.body, ul);
    hideList();
    addEvent(ul);
  });
})(window, document);
<input type="text" placeholder="type to search" id="typeahead">

\$\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.