1
\$\begingroup\$

I have this example data :

 var objects = [
                {
                    title: "Parent Area",
                    code: "P",
                    areas: "B,C",
                    child: [],
                    selected: 0,
                    remove: 0
                },
                {
                    title: "Child area one",
                    code: "B",
                    areas: "C",
                    child: [],
                    selected: 0,
                    remove: 0
                },
                {
                    title: "Child area two",
                    code: "C",
                    areas: undefined,
                    child: [],
                    selected: 0,
                    remove: 0
                }
            ];

I want to create this data set :

sortedSet = [
                {
                    title: "Parent Area",
                    code: "P",
                    areas: "B,C",
                    child: [
                        {
                            title: "Child area one",
                            code: "B",
                            areas: "C",
                            child: [
                                {
                                    title: "Child area two",
                                    code: "C",
                                    areas: undefined,
                                    child: [],
                                    selected: 0,
                                    remove: 0
                                }
                            ],
                            selected: 0,
                            remove: 0
                        },
                        {
                            title: "Child area two",
                            code: "C",
                            areas: undefined,
                            child: [],
                            selected: 0,
                            remove: 0
                        }

                    ],
                    selected: 0,
                    remove: 0
                }
            ];

Iv written the following code:

 /**
             * for each area get it's children
             */
            angular.forEach(objects, function (obj) {

                if (obj.areas) {

                    var subChildren = obj.areas.split(",");

                    /**
                     * assign the current child as child of the parent area
                     */
                    angular.forEach(subChildren, function (currentChild) {

                        angular.forEach(objects, function (currentObj) {

                            if (currentObj.code == currentChild) {

                                /**
                                 * mark this for removal from main array
                                 * @type {number}
                                 */
                                currentObj.remove = 1;
                                obj.child.push(currentObj);
                            }
                        })

                    })

                }

            })

            var sortedSet = [];
            /***
             * clean out main areas array
             */
            angular.forEach(objects, function (obj, index) {

                if (!obj.remove) {
                    /**
                     * copy the correct values to a secondary array
                     */
                    sortedSet.push(objects[index]);
                }
            })

My code works as required but I really wish I didn't need to do so many loops.

Any suggestion re-factors to optimise this code?

\$\endgroup\$
0

2 Answers 2

2
\$\begingroup\$

The number of nested loops can be reduced and doing so provides an opportunity to check your data for duplicate ids. This may not be necessary if you trust your source to provide good data.

var objectByCode={};

objects.forEach(function(o){
    // if( objectByCode[o.code] ) throw 'Duplicate codes';
    objectByCode[o.code]=o;
});

objects.forEach(function(o){
    if(o.areas){
        o.areas.split(",").forEach(function(c){
            if(objectByCode[c]){
                o.child.push(objectByCode[c]);
                objectByCode[c].remove=1;
            } // else { throw 'Code not found'; }
        });
    }
});

var sortedSet=objects.filter(function(o){ return !o.remove; });

Personally I would worry less about how many nested loops you have used and more about the potentially recursive data structure you are creating. This may prove difficult to work with if any of the children turn out to be their own ancestors. If for example the object with code C had area P then the resulting sortedSet would be empty. If B was a child of C then any naive attempt to list all descendants of B would result in a infinite loop.

If you are sure that your data will never contain this situation then you should be safe. If you are worried that this problem might occur a quick and dirty way to test for it is to use JSON.stringify which will throw an error when asked to produce the string equivalent of recursive objects.

try{
    JSON.stringify(sortedSet);
}catch(e){
    // how you decide to handle this is up to you
}

It might not be important to your application if this is the case, but be aware that your code may need to handle these situations.

\$\endgroup\$
0
\$\begingroup\$

I think Bob said the key word here.. reduce. I'm not a fan of using forEach or each because in most cases you can get away with using map or reduce, which I find safer to use because it allows you to manage mutation. Having said that, using for or while loops will be more efficient in terms of performance.

You can achieve what you want using a bit of recursion, and map/reduce. I figured I'd throw in my 2 cents.

Code & Demo

var objects = [{
  title: "Parent Area",
  code: "P",
  areas: "B,C",
  child: [],
  selected: 0,
  remove: 0
}, {
  title: "Child area one",
  code: "B",
  areas: "C",
  child: [],
  selected: 0,
  remove: 0
}, {
  title: "Child area two",
  code: "C",
  areas: undefined,
  child: [],
  selected: 0,
  remove: 0
}];

function matchPropertyValue(property, value, self) {
  return function(other) {
    if (self) {
      return other !== self && other[property] === value;
    }
    return other[property] === value;
  };
}

function findChildren(parent, context) {
  if (parent.areas) {
    var areas = parent.areas.split(",");

    var children = areas.reduce(function(children, area) {
      var childrenInArea = context.filter(matchPropertyValue('code', area, parent));
      return children.concat(childrenInArea);
    }, []);

    parent.child = children.map(function(child) {
      child.remove = 1;
      return findChildren(child, context);
    });
  }

  return parent;
}

var parentChildList = objects.map(function(element, index, context) {
  return findChildren(element, context);
}).filter(matchPropertyValue('remove', 0));

document.getElementById("result").innerText = JSON.stringify(parentChildList, null, 2);
<pre id="result"></pre>

Explanation

matchPropertyValue

function matchPropertyValue(property, value, self) {
  return function(other) {
    if (self) {
      return other !== self && other[property] === value;
    }
    return other[property] === value;
  };
}

This code basically creates a predicate to match items given a property and value. If self is provided, then it verifies that whatever you are comparing to is not the same thing. Creating generalized code allows for re-use, as you'll see we'll use this method in two places.

findChildren

function findChildren(parent, context) {
  if (parent.areas) {
    var areas = parent.areas.split(",");

    var children = areas.reduce(function(children, area) {
      var childrenInArea = context.filter(matchPropertyValue('code', area, parent));
      return children.concat(childrenInArea);
    }, []);

    parent.child = children.map(function(child) {
      child.remove = 1;
      return findChildren(child, context);
    });
  }

  return parent;
}

This function finds all the children for a parent given a list of items - called the context. Using reduce we can isolate all children that this belong to the parent's area. Using the matchPropertyValue predicate generator, we can easily match all children by area.

Once we have the list of children, we can find all descendants for these using recursion.

Generating the new list

var parentChildList = objects.map(function(element, index, context) {
  return findChildren(element, context);
}).filter(matchPropertyValue('remove', 0));

We can use the matchPropertyValue function again to match all items not to be removed.

Conclusions

There are a couple of things that I think need to be improved in the original code:

  • Create small reusable components. I'm sure findChildren can be refactored to be much more general to be reusable. In this case, at least, you end up with matchPropertyValue, which definitely can be reused all over the place.
  • Minimize state. To be honest, I haven't figured out how to solve your problem without the remove field. I do believe a solution that doesn't require this field will produce much more maintainable code. Hopefully, someone can come up with a better solution without the required remove state.
  • If you need to indent your code a lot, something can be refactored out. This isn't a hard rule, so use best judgement. As Linus Torvalds says:

If you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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