Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm looking for a review of the following code.

  1. Is there a better way to remove all properties that evaluate to truthy for a given function?
  2. Is there a better way to prevent a stack overflow in the recursive function?

This is the entire Node.js module:

// The removePropertiesWhereTruthy function accepts the following parameters:
// 
// 1. an object or array
// 2. a function to call against each non-object or non-array to determine if it's a truthy value according to that function (e.g. _.isNull)
// 3. An optional maxRecursion value to prevent a stack-overflow when there's a circular reference or exceptionally deeply nested objects.
// 
'use strict';

var _  = require('lodash');

function removePropertiesWhereTruthy(obj,func,maxRecursion) {

  maxRecursion = _.isNumber(maxRecursion) ? maxRecursion - 1 : 20;
  if(maxRecursion < 0) {
    // prevent stack overflow by allowing caller to set a max time for
    // recursion and default to 20 if not set.
    return;
  }
  if(_.isObject(obj) && !_.isArray(obj)) {
    _.forOwn(obj, function(v, k) {
      if(_.isObject(v)) { // also true for array
        return removePropertiesWhereTruthy(v,func,maxRecursion);
      } else if(func(v)) {
        delete obj[k];
      }
    });
  } else if(_.isArray(obj)) {
    _.remove(obj, function(item){
      return func(item);
    });
    _.each(obj, function(v){
      if(_.isObject(v)) { // also true for (nested) array
        return removePropertiesWhereTruthy(v,func,maxRecursion);
      }
    });
  } else {
    throw new Error('This function should only be called with objects and arrays');
  }
}

function removePropertiesWithNullValues(obj, maxRecursion) {
  return removePropertiesWhereTruthy(obj, _.isNull, maxRecursion);
}

module.exports = {
  removePropertiesWithNullValues: removePropertiesWithNullValues
};
share|improve this question
    
Could you explain what the code does in a bit more detail? What kind of input it accepts? What does it return? –  Madara Uchiha Jan 12 at 13:51
    
@MadaraUchiha - I added a comment about the parameters. The code removes any property with a value that evaluates to truthy based on the func parameter. It does not return anything as it manipulates the object and its properties in place. –  Guy Jan 12 at 14:23

1 Answer 1

I can't think of any good use-case for this method. It may help to describe some examples of why you need this functionality.

That said, a few issues I see:

  1. func could be named better (predicate perhaps?).
  2. maxRecursion seems a bit unstable. This is where I have a hard time seeing the purpose of this method. What happens if you hit level 20 and it hasn't fully run it's course? Now you've got an object that is in a half-complete state without any way to know it. What if it spends 20 cycles on the first property which happens to be a circular reference?
  3. I think the use of lodash here adds confusion to what would otherwise be pretty straightforward javascript (opinion).

Some fixes:

  1. Don't use a recursive function. You could use a stack or a queue here to traverse the object tree.
  2. Rather than limit the depth to avoid stack overflows for circular references, you could try to detect them by keeping a table of references and avoid re-traversing those trees altogether.
  3. Rather than modifying an existing object, you could create a new one. That way, this method wouldn't produce any side-effects and potentially leave you in an unstable state. At minimum, you could return true/false if the method succeeded.
share|improve this answer
    
Thanks for the comments. I like all your fixes. I was look at fix #3 and was wondering if I could achieve all this functionality through lodash's cloneDeep? var clone = _.cloneDeep(view, function(value) { return _.isElement(value) ? value.cloneNode(true) : undefined; }); I was wondering if the function passed into _.cloneDeep could somehow act as the predicate to achieve the copy without null values for example. –  Guy Jan 13 at 5:05
    
The documentation isn't super clear on how the callback in cloneDeep is used, but it would appear that it would do exactly what you need. You could just replace the _.isElement(value) line with yourPredicate(value). –  Ocelot20 Jan 13 at 14:15

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.