2
\$\begingroup\$

I've just refactored some code for a search box which takes a user search term, hits an API, returns a list of possible locations, and processes the users chosen result by sending them to a map page with their chosen result populated in the URL. I'm trying to improve my JS skills from just getting stuff done, to doing it well and to a high standard.

Any feedback greatly welcomed!

// requires: src/helpers.js

(function(document, undefined) {
    'use strict';

    /**
     * Create new helper object for use of helper functions
     * @type {Object}
     */
    var helpers = new Code.Helpers();

    /**
     * ID of the visit search form
     * @type {String}
     */
    var searchFormId = 'VisitSearchBox';

    /**
     * Container class for the multiple location dropdown
     * @type {String}
     */
    var multipleLocationClass = 'p-hero__search-box-results-list';

    /**
     * Class for multiple location dropdown items
     * @type {String}
     */
    var multipleLocationItemClass = 'p-hero__search-box-results-item';

    /**
     * Data attribute string for the multiple location dropdown list
     * @type {String}
     */
    var multipleLocationDropdownAttr = 'data-searchwoods-location-list';

    /**
     * Data attribute selector for multiple location dropdown
     * @type {String}
     */
    var multipleLocationDropdown = '[' + multipleLocationDropdownAttr + ']';

    /**
     * API query URL to return locations by term
     * @type {String}
     */
    var queryUrl = '/api/maps/locationsearch/';

    /**
     * URL where map results page lives
     * @type {String}
     */
    var woodsUrl = '/visiting-woods/map/';

    /**
     * Validation error class
     * @type {String}
     */
    var validationClass = 'input-validation-error';

    /**
     * Validation error text
     * @type {String}
     */
    var validationText = 'Please enter a location';

    /**
     * Is valid search term boolean
     * @type {Boolean}
     */
    var isValid = true;

    /**
     * Default radius used for map results
     * @type {Number}
     */
    var radius = 5;

    /**
     * Search box data attribute identifier
     * @type {String}
     */
    var searchBox = '[data-searchwoods-box]';

    /**
     * Search fieldset data attribute identifier
     * @type {String}
     */
    var searchFieldset = '[data-searchwoods-fieldset]';

    /**
     * Search box submit button identifier
     * @type {String}
     */
    var searchSubmit = '[data-searchwoods-button]';

    /**
     * Remove any previous dropdown results and clear errors
     */
    function resetSearchBox() {
        // Remove multiple list dropdown when user starts to search again
        if(helpers.$(multipleLocationDropdown)) {
            helpers.remove(helpers.$(multipleLocationDropdown));
        }

        // If the search input was in an invalid state, clear the error
        if (!isValid) {
            clearError();
        }
    }

    /**
     * Cancel default form submission behaviour and trigger custom search behaviour
     * @param  {object} e [submission event]
     */
    function cancelAndSearch(e) {

        // Stop the default form submission behaviour
        e.preventDefault();

        // Get and format the search term from the search box
        var searchTerm = escape(helpers.$(searchBox).value.trim());

        // Check search term is valid and display error if not
        if (searchTerm === 'undefined' || searchTerm === '' || searchTerm === validationText || searchTerm === 'Please%20enter%20a%20location') {
            displayError();
            return;
        }

        // Generate API search URL by combining the API query URL with the user search term
        var searchUrl = queryUrl + searchTerm;

        // Do an AJAX request to the API with the search URL
        ajaxSearchRequest(searchUrl, searchTerm);
    }

    /**
     * Perform an AJAX request to search for locations from the API based on the user search term
     * @param  {String} searchUrl [URL to request, based on the query URL and search term]
     * @param  {String} searchTerm [User location search term]
     */
    function ajaxSearchRequest(searchUrl, searchTerm) {

        // // Abort any open requests
        if (request) request.abort();

        // Use search URL to make request to search API
        var request = new XMLHttpRequest();
        request.open('GET', searchUrl, true);

        request.onload = function() {
            if (request.status >= 200 && request.status < 400) {
                var data = JSON.parse(request.responseText);
                request = null;

                // If the API returns multiple results then display multiple results
                if (data.hasMultiple) {
                    displayMultipleSearchResults(data.results, searchUrl);
                }
                // Else execute search and go to results page
                else {
                    var latlon = '';
                    if (data.results.length > 0) {
                        var lat = data.results[0].coords.lat;
                        var lon = data.results[0].coords.lon;
                        latlon = lat + '/' + lon;
                    }
                    showNearbyWoods(searchTerm, latlon);
                }
            }
            else {
                console.log('search data fail: ' + request.responseText);
            }
        };

        request.onerror = function() {
          console.log('search data fail: ' + request.responseText);
        };

        request.send();
    }

    /**
     * Display multiple search results
     * @param  {Object} data      [Returned AJAX data from API]
     * @param  {String} searchUrl [The search URL built from query URL and user search term]
     */
    function displayMultipleSearchResults(data, searchUrl) {
        var resultItems = [];
        var latlon;

        for (var i = 0; i < data.length; i++) {
            latlon = data[i].coords.lat + '/' + data[i].coords.lon;
            // Build results array with each returned result item
            resultItems.push('<span class="' + multipleLocationItemClass +'"><a href="' + searchUrl + '" data-url="' + searchUrl + '" data-coords="' + latlon + '">' + data[i].name + '</a></span>');
        }

        // Populate multiple location dropdown and show it
        var multipleLocationDropdownElement = document.createElement('div');
        multipleLocationDropdownElement.setAttribute(multipleLocationDropdownAttr, '');
        multipleLocationDropdownElement.classList.add(multipleLocationClass);
        multipleLocationDropdownElement.innerHTML = resultItems.join('');
        helpers.$(searchFieldset).appendChild(multipleLocationDropdownElement);
        var multipleLocationDropDownHeight = getComputedStyle(multipleLocationDropdownElement).height;
        helpers.$(multipleLocationDropdown).classList.add(multipleLocationClass + '--active');

        // Add event listener to the dropdown
        helpers.$(multipleLocationDropdown).addEventListener('click', processResultLink);
    }

    function processResultLink(e) {
        e.preventDefault();

        // Check if the clicked element was a link
        if (e.target !== e.currentTarget) {
            var chosenResultText = e.target.text;
            var chosenResultUrl = e.target.href;
            var chosenResultTerm = chosenResultText;
            var latlon = e.target.dataset.coords;

            // Remove the location dropdown
            var multipleLocationDropdownElement = helpers.$(multipleLocationDropdown);
            multipleLocationDropdownElement.parentNode.removeChild(multipleLocationDropdownElement);
            helpers.$(searchBox).value = chosenResultText;

            showNearbyWoods(chosenResultTerm, latlon);
        }
        e.stopPropagation();
    }

    /**
     * Build URL to send user to and send user
     * @param  {String} locationTerm [The location term determined by the user location selection]
     * @param  {String} latlon       [The latitude and longitude of the location]
     */
    function showNearbyWoods(locationTerm, latlon) {

        var nearbyWoodsUrl = woodsUrl + locationTerm + '/' + radius + '/';

        if (latlon) {
            nearbyWoodsUrl = woodsUrl + locationTerm + '/' + latlon + '/';
        }

        window.location = nearbyWoodsUrl;
        return;
    }

    /**
     * Display an error on the search input box
     */
    function displayError() {
        isValid = false;
        helpers.addClass(helpers.$(searchBox), validationClass);
        helpers.$(searchBox).blur();
        helpers.$(searchBox).value = validationText;
    }

    /**
     * Clear any errors on the saecrh input box
     */
    function clearError() {
        helpers.$(searchBox).value = '';
        helpers.removeClass(helpers.$(searchBox), validationClass);
        isValid = true;
    }

    /**
     * Initiate. Reset the search box and add event listeners
     */
    function init() {
        resetSearchBox();
        helpers.$(searchBox).addEventListener('focus', resetSearchBox);
        helpers.$(searchBox).addEventListener('keyup', resetSearchBox);

        // Trigger search function on search box submission
        helpers.addEvent(helpers.$('#' + searchFormId), 'submit', cancelAndSearch);
    }

    init();

})(document);
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

A few thoughts:

  • Do you really want to define this class as in IIFE? You are potentially limiting re-use of the components. What you in essence have here is a class. A class that you could re-use across multiple applications. A class that could have multiple instances operating on a page to interact multiple search boxes and endpoints. You should consider giving this class a name and truly treating it like an instantiable object in your system.

Example:

// class that could be included in file.
// class definition and constructor
function ajaxSearch (config, callbacks) {
    // instance variables
    var config = $.extend(this.defaultConfig, config);
    var callbacks = $.extend(this.defaultCallbacks, callbacks);


    // some constructor logic
    function init() { ... };   
    init();
};

// class-levels methods
// most of your methods live on prototype, not on instance
ajaxSearch.prototype = {
    'method1': function() {},
    // etc.    
}

// "static" prototype variables common to all object instances
// for example for defaults, common dependencies, etc.
ajaxSearch.defaultConfig = { ... };
ajaxSearch.defaultCallbacks = { ... };
ajaxSearch.codeHelpers = new Code.Helpers();


// usage on page
$ajaxSearch1 = new ajaxSearch(config1, callbacks);
$ajaxSearch2 = new ajaxSearch(config2, callbacks);
  • Given my example above, you will note two things I am doing that make this approach more re-usable. I am passing in configuration to the class, and I am passing in callbacks. If config holds most of your element selector information, it becomes easy to change class names, id's, etc. for different usage contexts in different applications. You can also pass in configuration for API endpoint and many of the other variables you have on your class.

By passing in callbacks, you give yourself flexibility to define different DOM interactions all triggered by hooks on steps in the ajax request process. You can define the hooks in your class for different parts in the process - request initiation, request failed, request success, DOM insertion, etc.

You would have the constructor potentially fully replace or extend default configurations that are stored on the class statically. The constructor could also store all the callbacks on the object instance. Since you are using jQuery, you might consider extend() for this purpose.

  • You should strongly consider caching element references. There should be no reason to continually re-query the DOM with every method invocation. My suggestion would be to, upon instantiation, in a method called by your constructor, use selector configuration to perform all necessary DOM queries. And store references to those jQuery collections on this instance. These stored collections would be available to all your class methods and callbacks to operated against. This may also present an opportunity for you to make sure that all the appropriate elements are found on the page before you can actually begin working with the ajaxSearch object (you would throw errors or similar for failures). After set-up, this will greatly improve performance of your methods and callbacks, especially when you are using thing like data attribute selectors which can be expensive when querying the DOM.

  • This approach would obviously mean some significant refactoring, with you removing a lot of the DOM interactions out of the class and into callbacks, so I haven't really spent a lot of time in looking over your methods in their curent state.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.