0

I have made a multi select dropdown which is implemented like below 

 MyFilter{
      key: string;      // uniquely identifies selected filter among many filters on page
      order: number;    // display order of filter on page
      options: DropListOption[];  // options in a dropdown
      values: DropListOption[];   // selected values from dropdown
    }  

 DropListOption{
    label: string;  // name of option
    value: any;     //value of option
   }

I have implemented a method which is called when a checkbox is selected or unselected as below:

onSelect(key: string, option: DropListOption): void{
  const filtersOpt = this.filters;  // type of filter is MyFilter
  filtersOpt.map((filter) => {
    if(filter.key === key){
      const selected: boolean = !!filter.values.find((opt) => opt.value === option.value);
      filters.values = selected  // filters.values used as property binding to display number of selected values
                      ? filters.values.filter((opt) => opt.value !== option.value)
                      : [...filters.values, option];
    }
    return filter;
  });
  this.updateFilters.emit(filtersOpt); //emitting to parent component
}

Although my logic is working fine but being new to Angular/RxJS (front-end development), I am not very sure if my code is good code. Please can you let me know our thoughts or correct me. I would appreciate if someone can tell me any better way of implementing my onSelect() method.

Akash Sharma
  • 330
  • 3
  • 18

1 Answers1

1

I'm not exactly sure of any improvements that could be done in your html to simplify this method any further, since you didn't show it, but blatantly speaking I'd do as follows.

Solution #1

This solution is not compatible with Internet Explorer due to the Array.prototype.findIndex() method usage. See Solution #2 for a compatible solution.

onSelect(key: string, option: DropListOption): void {
    const filterOpt = this.filters.find(fil => fil.key === key);
    if (!filterOpt) {
        console.warn('No filter was found with the given key.');
        return;
    }

    const selectedOptIdx =  filterOpt.values.findIndex(opt => opt.value === option.value);

    if (selectedOptIdx > -1) {
        filters.splice(selectedOptIdx, 1);
    } else {
        filters.push(option);
    }
}

Pros

  • less complex solution.
  • overall, has a slightly better readability.

Solution #2

onSelect(key: string, option: DropListOption): void {
    const filterOpt = this.filters.find(fil => fil.key === key);
     if (!filterOpt) {
        console.warn('No filter was found with the given key.');
        return;
    }

    const selectedOpt = filterOpt.values.find(opt => opt.value === option.value);

    if (selectedOpt) { 
        filters.splice(filters.indexOf(selectedOpt), 1);
    } else {
        filters.push(option);
    }
}

Pros

  • compatible with Internet Explorer.

Overall, I would say that there are a few better approaches that can be done, I would advise you to read about the complexity of some Array.prototype methods so you can have a better understanding of the choices you make along the way and try to understand better what the Array.prototype.map() method does, in particular. I'll leave the sources below.

Sources

  • performance comparison between splice and filter (and copyWithin). (link)
  • discussion regarding javascript's array methods' complexities. (link)

Array.prototype.map() from MDN documentation pages (link)

(...) map calls a provided callback function once for each element in an array, in order, and constructs a new array from the results. callback is invoked only for indexes of the array which have assigned values (including undefined). (...)

Gonçalo Alves
  • 36
  • 5