6
\$\begingroup\$

Having some trouble with a code I'm trying to refactor, where I need to go over a 'tree' of items, filter some invalid items, and create a new array of filtered items which takes properties from both parent and child objects.

To demo the problem I made a quick snippet -

const items = [
  {
    name: 'Shirley Hardin',
    isValid: true,
    children: [
      { 
        name: 'Mildred Mata',
        isValid: true,
      },
      { 
        name: 'Christopher Herring',
        isValid: false,
      },
    ],
  },

  {
    name: 'Tad M. Colbert',
    isValid: false,
    children: [
      { 
        name: 'James J. Spencer',
        isValid: true,
      },
    ],
  }
];

function toParentChildModel(parent, child) {
  return {
    parentName: parent.name,
    childName: child.name,
  };
}

function getValidChildParent(list) {
  const validParentChild = [];
  for (let i=0; i<list.length; i++) {
    const parent = list[i];
    if (parent.isValid) {
      for (let j=0; j<parent.children.length; j++) {
        const child = parent.children[j];
        if (child.isValid) {
          validParentChild.push(toParentChildModel(parent, child));
        }
      } 
    }
  }
  return validParentChild;
}

function print(list) {
  console.log(list);
}

print(getValidChildParent(items));

The code can also be found at here

How would you refactor this code, specifically the 'getValidChildParent' function, in a functional way?

\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

getValidParentAndChild can be rewritten as follows:

function getValidParentAndChild(list){
    return list.filter((obj) => obj.isValid)
               .map((obj) => obj.children.filter((kid) => kid.isValid)
                                         .map((kid) => toParentChildModel(obj, kid))
               .reduce((arr1, arr2) => arr1.concat(arr2));
}

The filter takes the element of the list and keeps it if the condition passed in is met. (In this case, both uses keep all the valid elements.)

map applies a function over each element of the list and returns the list of what the function returned for each element. Here it converts the list to a list of the parent-child pairs.

reduce is also called fold. These apply a two-argument over a list such that if your list was [a, b, c, d], with the function f, the reduce would return f( f( f(a, b), c), d). Here it is used to make a 2D list a 1D array.

This problem becomes more interesting if children could have children. In that case I would do the following:

function getPairsForNode(node){
    if(node.children)
       return node.children.map((child) => getPairsForNode(child))
                           .concat(node.children.map((child) => toParentChildModel(node, child))
                           .reduce((arr1, arr2) => arr1.concat(arr2));
    else return [];
}

function getValidParentAndChild(list){
    return list.filter((node) => node.isValid)
               .map(getPairsForNode)
               .reduce((arr1, arr2) => arr1.concat(arr2));
}
\$\endgroup\$
3
\$\begingroup\$

Interesting question,

since Heman addressed what you wanted to have reviewed, I will take a stab at the rest:

  • isValid seems like a very bad naming choice, having it on both parent and child does not help at all. There must be some logic to determine the validity and that logic should be reflected in the name of the variable.

    • If however, that is really the way it should be, then you could enhance a tiny bit Heman's code by having a dedicated isValid function.

      function isValid( o ){
        return o.isValid;
      }
      
      function getValidParentAndChildNames(list){
        return list.filter(isValid)
                   .map((parent) => parent.children.filter(isValid)
                                                   .map((child) => toParentChildModel(parent, child))) 
                   .reduce((previous, current) => previous.concat(current))
      }
      
  • I would also suggest that getValidParentChildNames is a better name than getValidParentAndChild, since you are getting an array back (ends with s) and since you are only getting the names back, not the actual nodes

    • If you follow that logic thru, then consider also renaming toParentChildModel model, and then you can even consider passing the names straight to the function (why should the function only work for your model? If your function got the names straight, then it would be far more re-usable) The final value would be that you could then use a great ES6 feature:

       return {
         parentName,
         childName,
       };
      
\$\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.