3
\$\begingroup\$

The function filterList recieves an array of objects and filters the list, checking only the properties of the object specified in a 'filterFields' array.

It compares the values of those specified properties to a string 'filterTerm' using a comparison function.

If the property specified by 'filterFields' is an array or object, it also checks through those and returns the entire top-level object if the specified fields of any child objects are matched with the comparison function.

I would like to check to see if this is normal/sensible javascript and whether there is a better way of writing this functionality, performance-wise.

    //if filterTerm or filterFields are empty, return original list
    function filterList(list, filterTerm, filterFields) {
        if(filterTerm && filterFields.length > 0) {
            return list.filter(filterCallback(filterFields, filterTerm));
        }else{
            return list;
        }
    }
    //use closure to pass filterFields and filterTerm to callback
    function filterCallback(filterFields, filterTerm) {
        return function(element) {
            return filterFields.some((filterField)=>{
                if(Array.isArray(element[filterField])){
                    return element[filterField].some(function(childElement){
                        return filterCallback(childElement);
                    })
                }else if(typeof element[filterField] === 'object'){
                    return filterCallback(element[filterField]);
                }
                return comparisonFunction(element[filterField], filterTerm);
            })
        }
    }
    //comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
    function comparisonFunction(fieldValue, filterTerm) {
        return fieldValue &&
        (String(fieldValue).toLowerCase().indexOf(filterTerm.toLowerCase()) >= 0
        || String(filterTerm).toLowerCase().indexOf(fieldValue.toLowerCase()) >= 0);
    }

//EXAMPLE USE:
    arr = [{name: "a", job: "b"}, {name:"c", job:"d"}];
    filterList(arr, "a", ["name"]); // => [{name: "a", job:"d"}]
    arr2 = [{name: "a", job: "b", employees: [{name: "d", job:"e" }]}, {name: "f", job: "g"}];
    filterList(arr2, "d", ["name", "employees"]); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

```
\$\endgroup\$
0
3
\$\begingroup\$

DRY your code You reference element[filterField] quite a few times in filterCallback. It would be nicer to save the value in a standalone variable first - then you can reference that variable instead of doing object property lookup each time.

Nested object bug filterCallback returns a function, but here, if an array or object is found, you're returning a call of filterCallback, which will always be truthy, so the .some will pass, even when it shouldn't:

return filterFields.some((filterField) => {
  if (Array.isArray(element[filterField])) {
    return element[filterField].some(function(childElement) {
      return filterCallback(childElement); // <--- always truthy
    })
  } else if (typeof element[filterField] === 'object') {
    return filterCallback(element[filterField]); // <--- always truthy
  }
  return comparisonFunction(element[filterField], filterTerm); // <--- OK
})

// No items should pass the test, but they do:



//if filterTerm or filterFields are empty, return original list
function filterList(list, filterTerm, filterFields) {
  if (filterTerm && filterFields.length > 0) {
    return list.filter(filterCallback(filterFields, filterTerm));
  } else {
    return list;
  }
}

//use closure to pass filterFields and filterTerm to callback
function filterCallback(filterFields, filterTerm) {
  return function(element) {
    return filterFields.some((filterField) => {
      if (Array.isArray(element[filterField])) {
        return element[filterField].some(function(childElement) {
          return filterCallback(childElement);
        })
      } else if (typeof element[filterField] === 'object') {
        return filterCallback(element[filterField]);
      }
      return comparisonFunction(element[filterField], filterTerm);
    })
  }
}
//comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
function comparisonFunction(fieldValue, filterTerm) {
  return fieldValue &&
    (String(fieldValue).toLowerCase().indexOf(filterTerm.toLowerCase()) >= 0 ||
      String(filterTerm).toLowerCase().indexOf(fieldValue.toLowerCase()) >= 0);
}

arr2 = [{
  name: "a",
  job: "b",
  employees: [{
    name: "z",
    job: "e"
  }]
}, {
  name: "f",
  job: "g"
}];
console.log(filterList(arr2, "d", ["name", "employees"])); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

You need to call filterCallback(filterFields, filterTerm) first to get the filter function, then call the filter function with the value to check, eg:

return filterFields.some((filterField) => {
    const value = element[filterField];
    if (Array.isArray(value)) {
        return value.some(function (childElement) {
            return filterCallback(filterFields, filterTerm)(childElement);
        });
    } else if (typeof value === 'object') {
        return filterCallback(filterFields, filterTerm)(value);
    }
    return comparisonFunction(value, filterTerm); // <--- OK
});

//if filterTerm or filterFields are empty, return original list
function filterList(list, filterTerm, filterFields) {
  if (filterTerm && filterFields.length > 0) {
    return list.filter(filterCallback(filterFields, filterTerm));
  } else {
    return list;
  }
}

//use closure to pass filterFields and filterTerm to callback
function filterCallback(filterFields, filterTerm) {
  return function(element) {
    return filterFields.some((filterField) => {
        const value = element[filterField];
        if (Array.isArray(value)) {
            return value.some(function (childElement) {
                return filterCallback(filterFields, filterTerm)(childElement);
            });
        } else if (typeof value === 'object') {
            return filterCallback(filterFields, filterTerm)(value);
        }
        return comparisonFunction(value, filterTerm); // <--- OK
    });
  }
}
//comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
function comparisonFunction(fieldValue, filterTerm) {
  return fieldValue &&
    (String(fieldValue).toLowerCase().indexOf(filterTerm.toLowerCase()) >= 0 ||
      String(filterTerm).toLowerCase().indexOf(fieldValue.toLowerCase()) >= 0);
}

//EXAMPLE USE:

arr = [{
  name: "a",
  job: "b"
}, {
  name: "c",
  job: "d"
}];
console.log(filterList(arr, "a", ["name"])); // => [{name: "a", job:"d"}]


arr2 = [{
  name: "a",
  job: "b",
  employees: [{
    name: "z",
    job: "e"
  }]
}, {
  name: "f",
  job: "g"
}];
console.log(filterList(arr2, "d", ["name", "employees"])); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

Precise variable names To reduce bugs, best to name variables as precisely as possible - filterCallback isn't actually the callback used in filter or some, but it's a function which creates such a callback. Maybe call it createFilterCallback instead - that probably would've made the problem clearer at a glance.

Implicit return Since you're using ES6, you could consider using arrow functions too - their implicit return can make code much more concise. Less code means less surface area for bugs, and can sometimes make it easier to read.

comparisonFunction If you want to check whether a string includes another, in ES6, better to use .includes(...) than .indexOf(...) >= 0. You also have a potential bug - you do String(fieldValue).toLowerCase() and String(filterTerm).toLowerCase(), implying that you expect that the value or term may not be a string - but then you do .indexOf(filterTerm.toLowerCase()) and .indexOf(fieldValue.toLowerCase()), which will throw if they aren't strings. Make sure to convert them to strings first:

const comparisonFunction = (fieldValue, filterTerm) => {
    const value = String(fieldValue).toLowerCase();
    const term = String(filterTerm).toLowerCase();
    return value && value.includes(term) || term.includes(value);
};

null is an object too, unfortunately, so:

}else if(typeof element[filterField] === 'object'){
    return filterCallback(element[filterField]);
}

will result in a recursive call, even when such a call won't make sense. The code works, but it's confusing flow. Better to only recurse if the value is an object and is not null.

filterCallback The whole filterCallback looks a bit ugly IMO. I'd simplify it a bit by not calling the whole function again recursively, but by declaring the function to filter elements by inside the function before returning it. Then, instead of recursing on filterCallback, recurse on that declared function:

const filterCallback = (filterFields, filterTerm) => {
  const testElement = element => filterFields.some((filterField) => {
    const value = element[filterField];
    if (Array.isArray(value)) {
      return value.some(testElement);
    } else if (value && typeof value === 'object') {
      return testElement(value);
    } else {
      return value && comparisonFunction(value, filterTerm);
    }
  });
  return testElement;
}

In all:

//if filterTerm or filterFields are empty, return original list
function filterList(list, filterTerm, filterFields) {
  if (filterTerm && filterFields.length > 0) {
    return list.filter(filterCallback(filterFields, filterTerm));
  } else {
    return list;
  }
}
//use closure to pass filterFields and filterTerm to callback
const filterCallback = (filterFields, filterTerm) => {
  const testElement = element => filterFields.some((filterField) => {
    const value = element[filterField];
    if (Array.isArray(value)) {
      return value.some(testElement);
    } else if (value && typeof value === 'object') {
      return testElement(value);
    } else {
      return value && comparisonFunction(value, filterTerm);
    }
  });
  return testElement;
}
//comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
const comparisonFunction = (fieldValue, filterTerm) => {
  const value = String(fieldValue).toLowerCase();
  const term = String(filterTerm).toLowerCase();
  return value.includes(term) || term.includes(value);
};

//EXAMPLE USE:
arr = [{name: "a", job: "b"}, {name:"c", job:"d"}];
console.log(filterList(arr, "a", ["name"])); // => [{name: "a", job:"d"}]
arr2 = [{name: "a", job: "b", employees: [{name: "d", job:"e" }]}, {name: "f", job: "g"}];
console.log(filterList(arr2, "d", ["name", "employees"])); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

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