current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

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?

share|improve this question
up vote 4 down vote accepted

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));
}
share|improve this answer
up vote 3 down vote

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,
 };
share|improve this answer
up vote 0 down vote

Thanks for all the answers, I ended up with this code:

function getValidChildParent(list) {
  const merge = arrays => [].concat.apply([], arrays);

  function isValid(item) {
    return item.isValid;
  }

  function scanParent(parent) {
    const toModel = child => toParentChildModel(parent, child);
    return parent.children.filter(isValid).map(toModel);
  }

  return merge(list.filter(isValid).map(scanParent));
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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