11
\$\begingroup\$

I'm trying to learn how to write better, more efficient JavaScript code and wanted to run this function I'd written by you and see if people can suggest an improved method.

It's basically a people filter based upon two variables.

Demo.

It works fine, but if additional categories come in (so far there are three) then the code would become redundant.

var family = 'director';
var expertise = 'asset';


function showConsultant(){ 
   $('.profile').stop(true,true).fadeOut(100);
   $('.profile[data-family="'+family+'"][data-expertise="'+expertise+'"]').delay(101).fadeIn();
}

showConsultant();

$('.top-level li').on('click', function() {

  $('.top-level li').removeClass('selected');
  $(this).addClass('selected');

  if( $(this).index() === 0 ){
     family = 'director';
     showConsultant();
  } else if( $(this).index() === 1 ){
      family = 'consultant';
      showConsultant();
  } else if( $(this).index() === 2 ){
      family = 'support';
      showConsultant();
  }

});

$('.second-level li').on('click', function() {

  $('.second-level li').removeClass('selected');
  $(this).addClass('selected');

  if( $(this).index() === 0 ){
     expertise = 'asset';
     showConsultant();
  } else if( $(this).index() === 1 ){
      expertise = 'insurance';
     showConsultant();
  } else if( $(this).index() === 2 ){
      expertise = 'wealth';  
     showConsultant();
  }

});
\$\endgroup\$
3
\$\begingroup\$

I stripped out most of what seemed unnecessary to still make the code do the same thing. By adding an array and using the index as your reference you can eliminate the need for the if, then, else clauses.

var family = 'director';
var expertise = 'asset';

var familyArray = ['director', 'consultant', 'support'];
var expertiseArray = ['asset', 'insurance', 'wealth'];

function showConsultant(){ 
   $('.profile').stop(true,true).fadeOut(100);
   $('.profile[data-family="'+family+'"][data-expertise="'+expertise+'"]').delay(101).fadeIn();
}

showConsultant();

$('.top-level li').on('click', function() {

  $('.top-level li').removeClass('selected');
  $(this).addClass('selected');
  family = familyArray[$(this).index()];
  showConsultant();

});

$('.second-level li').on('click', function() {

  $('.second-level li').removeClass('selected');
  $(this).addClass('selected');
  expertise = expertiseArray[$(this).index()];
  showConsultant();

});

You could probably go one step further and write the function handler for the event separately such that the same function could handle both click events for the top-level and second-level li's. You'd just have to do a check for which class the li falls under (i.e. top-level or second-level) and change the appropriate variable (i.e. family or expertise).

var family = 'director';
var expertise = 'asset';

var familyArray = ['director', 'consultant', 'support'];
var expertiseArray = ['asset', 'insurance', 'wealth'];

function showConsultant(){ 
   $('.profile').stop(true,true).fadeOut(100);
   $('.profile[data-family="'+family+'"][data-expertise="'+expertise+'"]').delay(101).fadeIn();
}

function clickHandler () {
  $(this).siblings().removeClass('selected');
  $(this).addClass('selected');
  $(this).parent().hasClass('top-level')
    ? family = familyArray[$(this).index()]
    : expertise = expertiseArray[$(this).index()];

  showConsultant();
}

showConsultant();

$('.top-level li').on('click', clickHandler);
$('.second-level li').on('click', clickHandler);
\$\endgroup\$
8
\$\begingroup\$

Make a lookup table. Then access by index:

var expertises = ["asset", "insurance", "wealth"];
expertise = expertises[$(this).index()];
showConsultant();

If you're worried about going out of bounds, add a range check:

var expertises = ["asset", "insurance", "wealth"];
var index = $(this).index();
if (0 <= index && index < expertises.length) {
    expertise = expertises[index];
    showConsultant();
}

You could even globally define these lists somewhere, so you could write enums.getExpertise(index).

\$\endgroup\$
2
\$\begingroup\$

The added comments explain my take on your code:

// Maybe rename this to something more intuitive
// This will allow you to add unlimited click combinations without additional logic
// This is the main piece which will help in avoiding redundancy
var list = [
    { // index 0
        family:'director',
        expertise:'asset'
    },
    { // index 1
        family:'consultant',
        expertise:'insurance'
    },
    { // index 2
        family:'support',
        expertise:'wealth'
    }
];

var family_index = 0;
var expertise_index = 0;

// Don't pollute the global space with functions
var showConsultant = function(){ 

    // Take advantage of the callback parameter of the fadeOut() function to guarantee the proper execution order
    // because .delay(101) might not fire off precisely after fadeOut() has finished
    $('.profile').stop(true,true).fadeOut(100, function(){

        // Call upon your list and the presently set indexes to figure out what to fadein()
        $('.profile[data-family="'+list[family_index].family+'"][data-expertise="'+list[expertise_index].expertise+'"]').fadeIn();
    });
};

showConsultant();

// Listen for clicks in either level and use .hasClass() to determine which level we are dealing with
$('.top-level li, .second-level li').on('click', function() {

    // $(this) is needed several times so just cache it
    var $this = $(this);
    var index = $this.index();
    var level = ($this.hasClass('top-level') ? 'top' : 'second');

    // Dynamically target the level
    $('.'+level+'-level li').removeClass('selected');
    $this.addClass('selected');

    // Make sure that what the user clicked is a defined choice in your list variable
    if( index < list.length ){

        if(level === 'top'){
            family_index = index;
        } else {
            expertise_index = index;
        }

        showConsultant();
    }

});
\$\endgroup\$
  • 3
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. It might help if you pull the comments out of the code so they're easier to read. \$\endgroup\$ – Pimgd Jul 19 '16 at 13:35
  • \$\begingroup\$ Basically, you have a good answer, but I can't easily tell what you changed and why. \$\endgroup\$ – Pimgd Jul 19 '16 at 13:52
  • 1
    \$\begingroup\$ Summarising you're code comments outside of the code block would make it much easier to identify your review aspects. At first glance, this looks a lot like a code dump. \$\endgroup\$ – forsvarir Jul 19 '16 at 13:54
  • 2
    \$\begingroup\$ @Pimgd Sorry, this is my first time answering on this site but when I have some additional free time then I will look into breaking out the code and comments properly. Thank you \$\endgroup\$ – MonkeyZeus Jul 19 '16 at 13:58
  • \$\begingroup\$ @MonkeyZeus ping me once you've done so with a comment and I'll take another look at the answer; if it's good I'll vote it up =) \$\endgroup\$ – Pimgd Jul 19 '16 at 14:01
2
\$\begingroup\$

It seems that what you are trying to implement is visual filtering of elements based on the selected values of various controls. Each control could filter on one possible filtering "dimension" for the elements - family, expertise, other things possibly in future.

For this reason, I do not understand the parent-child paradigm you are trying to apply here. I would think about the problem as being one of how to introduce an arbitrary control that will filter a set of elements on any number of filtering dimensions.

To achieve this, what may be the common code pieces that you need to have in place?

  • A handle to capture all the elements that are subject to filtering. In this case this might mean a jQuery selector which can represents all possible elements. I would suggest a common class. So rather than looking for things like *-level li, you simply select $('.filterableElement') and apply filterableElement class to each of these elements.. This also decouples filtering functionality from any HTML structural elements (i.e. ul, li, etc.)
  • A way to apply filtering control to a given set of elements (i.e. attach event handlers to trigger filtering).
  • A way to map each filtering control to the set of elements that it hides.
  • Code to actually implement the filtering (show/hide) operations.

If you have all this in place, you can scale this code to handle an arbitrary number of filtering controls.

What I propose below is a bit of a departure from your current approach, but wanted to present an approach to how one might approach the problem with the goals of re-usability (can this be used in more that one place?) and flexibility (can I use the same logic to address a number of use cases such as implementing an arbitrary set of filtering controls?). I will take the approach of implementing a class in javascript which can provide the functionality noted above.

My answer might seem overly verbose/complex to what you are starting with, but I wanted to give you a feel for how you might build this in a less fragile way, with your javascript being able to validate that HTML and data attributes are configured properly at the point of object instantiation.

Since this class has a dependency on jQuery you might even consider writing this as a jQuery extension (which should be quite easy, using this as a starting point). This overall sort of approach would be what I would expect if I were having an experienced javascript developer assigned to this coding task - that is if this is not a simple one-off (which doesn't sound like the case for you since you were asking about extensibility).

I have not made any attempts to abstract the implementation from the object caller (by adding privacy via instance methods for example), as, generally, attaching methods on the prototype is preferred to instance methods for performance. I usually just go with the fact that javascript isn't really designed like other object oriented languages with regards to privacy and only implement instance privacy when there is a strong case for it.

First let's take a look at how you may modify the HTML.

<!--
  You can continue to pass data attributes via HTML as you are
  currently doing. You might also consider just injecting configuration
  data directly into javascript vs. using data attributes, which
  might end up with cleaner implementation, but I am not going to
  wander into that area here.
  Here the suggestion is to attach a name for
  the filtering "dimension" to the root filterControl and pass
  data on the target values and related control that each option
  toggles.
  It is important to note that all these data values
  should likely be limited to names that meet standard
  javascript object property syntax requirements.
-->
<ul class="top-level filterControl" data-filter-name="family">
    <li
        class="filterOption"
        data-filter-target="directors"
        data-filter-control="family"
    >Directors</li>
    <li
        class="filterOption"
        data-filter-target="consultants"
        data-filter-control="family"
    >Consultants</li>
    <li
        class="filterOption"
        data-filter-target="support"
        data-filter-control="family"
    >Support</li>
</ul>
<ul class="second-level filterControl" data-filter-name="expertise">
    <li
        class="filterOption"
        data-filter-target="assetManagement"
        data-filter-control="expertise"
    >Asset Management</li>
    <li
        class="filterOption"
        data-filter-target="insurance"
        data-filter-control="expertise"
    >Insurance</li>
    <li
        class="filterOption"
        data-filter-target="wealthManagement"
        data-filter-control="expertise"
    >Wealth Management</li>
</ul>

<!--
    Here the filterable element may have any number of associated controls.
    Again passing all relevant filter options applicable to this element
    can be done via HTML data attributes, but this can begin to look
    a little odd.
    In this example we try to pass a valid JSON array representation
    with each array element being a string that indicates the filter
    dimension and option target value that this element relates to.
    This leverages jQuery.data() functionality that will automatically
    convert valid javascript literals (in this case an array literal)
    into the appropriate data structure as return to jQuery.data().
    I make the conscious decision here to put the data attribute within
    single quoted string to prevent need to escape quotes inside the string.
-->
<div
    class="profile filterableElement"
    data-filter-tags='["family::director","expertise::assetManagement"]'
>
    <span>James Emmet<br>director, asset</span>
</div>    

This again decouples display classes (like top-level) from behavioral ones, and would allow you to use any sort of elements - these could simply be divs or tr/td combinations - in any sort of layout. It doesn't matter, since our data attributes will tell the javascript class how to set itself up.

Next, let's focus on this class (we will call it FilterController). The thought here is that this class could be dropped on any page via reference/include, such that it is reusable anywhere in this application or other applications and is not tightly coupled with the display layout.

This class must be able to understand the different element types and provide a single place to implement the code logic pieces noted above. One thing we are going to look to do with this class is to minimize the need to re-query the DOM every time a filtering operation is needed. This class will store the jQuery selection references it needs to perform actions on DOM that result from filtering operations. This would greatly increase performance in cases where you are expecting the filtering controls to be heavily used.

// FilterController Class
// On class instantiation, we must pass the class names that will be used
// for different element types.  The class constructor logic builds
// everything it needs to implement filtering from these.
function FilterController(
    filterControlClass,
    filterOptionClass,
    filterElementClass
) {
    // validate passed parameters are as expected
    // here we use spread operator to convert arguments object to array
    this.validateClassParameters([...arguments]);

    // set passed class names
    this.filterControlClass = filterControlClass;
    this.filterOptionClass = filterOptionClass;
    this.filterElementClass = filterElementClass;

    // Object defining map to filter control elements.
    this.filterControls = {};

    // Store jQuery collection for all filter controls
    this.$filterControls = null;

    // Store jQuery collection for all filter options 
    this.$filterOptions = null;

    // Store Jquery collection for all filterable elements
    this.$filterableElements = null;

    // Object defining maps of controls to elements to be filtered.
    // this ultimately replaces querying the DOM with each click event
    // in favor of storing the mappings for optimized performance
    this.filteredElementTree = {};

    // Allow for setting of callbacks that allow caller to pass
    // in specific functions to execute during various filter operations
    // A default callback implementation is shown based on your hide/show
    // logic, but the intent is to allow user to override these to
    // whatever needs they have for their UI.
    // These callbacks are listed here in order callbacks are triggered
    // during filtering operation, though callbacks themselves would execute
    // asynchronously so there is no guarantee of callback completion order
    // One could guarantee synchronous execution of certain steps if needed.
    // That would require introducing Deferred/Promises or similar.
    this.callbacks = {
        // callback to execute on first receiving valid filter request
        // currently not implemented by default
        filterStart: function() {},

        // callback to execute when valid selection option has been made
        // we pass selected option DOM element and jQuery collection
        // of all option for that related filter control
        // this allows one to pass
        filterOptionSelected: function(el, $optionGroup) {
            // removed selected class all control options related
            // to control, which are passed here as jQuery collection
            $optionGroup.removeClass('selected');
            // add class to selected element
            $(el).addClass('selected');
        },

        // callback to execute when we have determined the currently
        // triggered filter control.  We pass this callback both the
        // selected control DOM element and jQuery collectoin of all
        // filter control elements so they can be operated against.
        // I didn't implement example call here as you didn't have
        // such behavior in your example. This could be something
        // similar to filterOptionSelected callback above.
        filterControlSelected: function(el, $filterControls) {},

        // callback to execute before filtering is applied.  This where
        // you would potentially reset filterable elements back to
        // their pre-filtered state.  We only execute this callback if
        // we know we have a valid filter control/option combination
        // specified so as not to change current screen display
        // under error conditions.
        // The callback expects a jQuery collection representing all
        // available filterable elements.
        resetFilters: function($allFilterableElements) {
            $allFilterableElements.stop(true,true).fadeOut(100);
        },

        // Callback to actually implement filtering on UI. This
        // callback expects jQuery collection representing elements.
        applyFilter: function($filteredElements) {
            $filteredElements.delay(101).fadeIn(); 
        },

        // callback to execute after filters have been applied
        // currently not implemented by default
        filterComplete: function() {}
    };

    // call methods to make this a functional object
    this.setFilterControls()
        .setFilterableElements()
        .buildFilteredElementTree();
}

// add methods to the FilterController class
FilterController.prototype = {

    // method used on instantiation to build all data structures
    // related to the filter controls and options into the object
    setFilterControls: function() {
        // get elements based on className
        $filterControls = $('.' + this.filterControlClass);
        // validate we have at least one filter control to work with
        if $filterControls.length === 0) {
            throw new Error('Unable to find any valid filter controls');
        }

        // store filter control collection on object
        this.$filterControls = $filterControls;   
        $filterControls.each(i, el) {
            // get filtering dimension name
            var filterName = this.getDataAttribute(el, 'filter-name');

            // build object to store control information.
            var control = {
                'domElement': el,
                'options': {},
                '$options': null
            };

            // Find all control options. Find all filter option
            // element which have this filter control as target        
            $controlOptions = $('.' + this.filterOptionClass +
                '[data-filter-control="' + filterName + '"]');

            // validate that we have at least one option related
            // to this filter
            if($controlOptions.length === 0) {
                throw new Error(
                    'No valid options found for control filter: ' + filterName;
                );
            }

            // set options jQuery collection on control object
            control.$options = $controlOptions;

            // iterate through options, setting info on control object
            $controlOptions.each(i, contEl) {            
                var target = this.getDataAttribute(contEl, 'filter-target');
                // we are not using the filter-control attribute here
                // but we want to go ahead and read it so that we can
                // validate it is properly configured 
                this.getDataAttribute(contEl, 'filter-control');
                control.options[target] = {
                    domElement: contEl
                };
            }

            // set control into filterControls map
            this.filterControls[filterName] = control; 
        }

        // return this for method chaining
        return this;
    },

    // method used upon instantiation to validate query collection
    // of filterable elements and set on object
    setFilterableElements: function() {
        // let's find all our filterable elements
        var $filterableElements = $('.' + this.filterElementClass);
        if ($filterableElements.length === 0) {
           throw new Error('No filterable elements found on page.');
        }
        // set jQuery collection on object
        this.$filterableElements = $filterableElements;

        return this;   
    },

    // method used on instantiation to build data structure
    // that maps filter controls and objects related filterable
    // elements and store that data structure on the object.
    buildFilteredElementTree: function() {
        // some local variable declarations for convenience
        var tree = this.filteredElementTree;
        var $elements = this.$filterableElements;

        // let's iterate through our filterable elements and map them
        // to their applicable controls
        $elements.each(i, el) {
            var data = this.getDataAttibute('filter-tags', el); 

            // iterate through each tag in data array
            for(var i = 0, length = data.length; i < length; i++) {
                var tagParts = data[i].split('::');
                // validate tag format
                if (tagParts.length !== 2) {
                    throw new Error(
                        "Invalid filter tag format. One '::' separator " +
                        "expected. Provided string: '" + data[i] + "'"
                    );
                }
                var dimension = tagParts[0];
                var optionValue = tagParts[1];
                // validate control dimension and value exist
                // and add to element tree if it does
                this.validateControlExists(dimension, optionValue);

                // if tree node does not yet exist we want
                // to place an empty array on that node
                if(!this.treeNodeExists(dimension, optionValue) {
                    // it does not, so we want to put empty array at that node
                    tree[dimension][optionValue] = [];
                }
                // put element into array at node
                tree[dimension][optionValue].push() = el;
            }
         }

         return this;
     },

     // Method to set custom callbacks.
     // We will use jQuery.extend() to overwrite any default handlers
     // with those passed.
     setCallbacks(callbacks) {
         $.extend(this.callbacks, callbacks);
     },

     // Main method to perform filtering activities.
     // This accepts DOM element representing the option that
     // has been selected in the UI
     filter: function(triggeredOptionElement) {         
         // trigger filterStart callback
         this.callbacks.filterStart();

         // based on the passed control option element we
         // get the appropriate dimension and option values
         // that will be used to perform filtering
         var filterSettings = this.getFilterSettings(triggeredOptionElement);
         var dimension = filterSettings.dimension;
         var optionValue = filterSettings.optionValue;

         var $optionGroup = this.filterControls[dimension].control.$options;

         // if we were able to retrieve filter settings that means
         // the passed element is a valid option control.
         // trigger option control selection callback
         this.callbacks.filterOptionSelected(
             triggeredOptionElement, $optionGroup
         );

         // trigger filterControlSelected callback
         this.callbacks.filterControlSelected(
             this.filterControls[dimension].domElement,
             this.$filterControls
         ); 

         // get filtered elements
         var filteredElementArray = this.getFilteredElements(
             dimension, optionValue
         );

         // we are now ready to update display
         // trigger resetFilters callback
         this.callbacks.resetFilters(this.$filterableElements);

         // trigger applyFilter callback
         this.callbacks.applyFilter($(filteredElementArray));          

         // trigger filterComplete callback
         this.callbacks.filterComplete();    
     },

     // helper method called during filtering to determine the
     // filter settings that are to be used based on data attributes
     // of the selected filter option.
     // This also validate that the passed element matches a known
     // filter option element in the object's data structure.
     getFilterSettings: function(el) {
         // validate that element is in list of option elements
         var $filteredOption = this.$filterOptions.filter(el);
         if ($filterOption.length !== 1) {
             throw new Error(
                 'Unable to find matching filter control for ' +
                 'passed option element.'
             );
         )
         // since we know this is a valid option element now,
         // we can read data attributes to determine dimension
         // and option value combination that we will use for filtering
         var filterSettings = {
             dimension: this.getDataAttribute(el, 'filter-control'),
             optionValue: this.getDataAttribute(el, 'filter-target')
         }
         return filterSettings;       
     }, 

     // method to get array of DOM elements that meet the passed
     // filtering criteria.  This method may also have some value
     // as a publicly available way to pass in string filter settings
     // to the class and get set of matching filterable elements
     getFilteredElements: function(dimension, optionValue) {
         // validate input
         if(!(
             this.isNonZeroLengthString(dimension) ||
             this.isNonZeroLengthString(optionValue)
         )) {
             throw new Error(
                 'Invalid parameter passed to FilterController' +
                 '.getFilteredElements(). ' +
                 'All parameters must be non-zero-length strings'
             )
         }
         // validate that the dimension and optionValue exist on tree
         this.validateTreeNodeExists(dimenstion, optionValue);

         return this.filteredElementTree[dimension][optionValue];
     },

     // generic handler for getting HTML data attributes from given element.
     // also performs some basic validation of attribute value
     getDataAttribute: function(element, key) {
         value = $(element).data(key);
         if(typeof value === 'undefined') {
             throw new Error(
                 "'" + key + "' data attribute is not set on element"
             );
         }
         // validate that want don't have an "emtpy" object to work with
         if(isNonZeroLengthString(value) || isNonZeroLengthArray(value)) {
             return value;
         }
         throw new Error(
             "Empty value provided for data attribute '" + key + "'"
         );
     },
     // Type verification and validation methods
     isNonZeroLengthString: function(value) {
         if(!(typeof value === 'string' || value instanceof String)) {
             return false;
         }
         if(value.length === 0) {
             return false;
         );
         return true;
     },

     isNonZeroLengthArray: function(arr) {
         if(!Array.isArray(arr)) {
             return false;
         }
         if(arr.length === 0) {
             return false;
         }
         return true;
     },

     validateClassNameParameters: function(argArray) {
        if(argArray.length !== 3) {
            throw new Error(
               'Invalid number of arguments passed at ' +
               'FilterContoller instantionation'
            );
        }
        argArray.forEach(function(value) {
            if(!this.isZonZeroLenthString) {
                throw new Error(
                    'All parameters passed to instantiate FilterController ' +
                    'must be strings of non-zero length'
                );
            }
        }
     },

     validateControlExists: function(dimension, optionValue) {
         if (typeof this.filterControls[dimension].options[optionValue]
             === 'undefined') {
             throw new Error(
                 'Filter tag without control set on element. ' +
                 'Filter dimension: ' + dimension + ' ' +
                 'Filter option value: ' + optionValue
             );
         }
     },  

     validateTreeNodeExists: function(dimension, optionValue) 
         if(!this.treeNodeExists(dimension, optionValue)) {
             throw new Error(
                 'Tree node request for filter does not exist. ' +
                 'Filter dimension: ' + dimension + ' ' +
                 'Filter option value: ' + optionValue
             );
         }
     },

     treeNodeExists: function(dimension, optionValue) {
         if (typeof this.filteredElementTree[dimension][optionValue]
             === 'undefined') {
             return false;
         }
         return true;
     }
}

Usage might look like this:

// Assumes filterController is already loaded as external script to page.
// wrap code execution in document ready handler
$(document).ready(function() {
    // instantiate filter controller with applicable class names
    var filterController = new FilterController(
        'filterControl', 'filterOption', 'filterableElement'
    );

    // Pass any custom callbacks here. This example will specify
    // override of filterStart callback (which has no default
    // implementation)   
    filterController.setCallbacks({
        filterStart: function() {
            console.log('Filter operation started.');
        }
    });

    // attach click handler for filter option elements
    // this will be how we pass selected option to filter
    // method.
    $('.filterOption').on('click', function() {
        filterController.filter(this);
    });
}
\$\endgroup\$
  • \$\begingroup\$ Wow. That is detailed; good answer. \$\endgroup\$ – Gary Storey Jul 28 '16 at 3:00

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.