Recursive function to copy over an object with very specific constraints

Question

I totally suck at recursion. I have a function that works but it also looks horribly wrong. I would love to 1) Know how to write it properly 2) Know about resources to read/study so that I don't get so stuck again.

The function is simple: given something like this:

  queryConditions = {
    type: 'and',
    args: [

      // First of all: video must be published or belonging to logged in user
      { type: 'or', args: [
        { type: 'eq', args: [ 'published', true ] },
        { type: 'eq', args: [ 'userId', '#_loggedInUser#' ] },
      ]},

      // Second: filter by userId if passed
      { type: 'eq', args: [ 'userId', '#userId#'] },


      { type: 'or', args: [
        { type: 'contains', args: [ 'title', '#s#' ] },
        { type: 'contains', args: [ 'videosTags.tagName', '#s#' ] },
      ] }

    ]
  },

It will return a copy of it, but:

• If ifDefined is set and the equivalent attribute in conditionsHashisn't set, the entry is skipped
• If a "leaf" has, as second field, something in this #format#, then its content is exchanged with conditionsHash['format'].
• if or or and only have one argument, then they are eliminated and replaced by their argument That means that you can't have { type: 'or', args: [ { type: 'eq', args: [ 'published', true ] } ]} since there is only one argument in or,

I am especially uneasy about the fact that I check twice if the type is 'and' or 'or'. I need to fully check this because I need to add a feature to this: I need to add the ability to add an each parameter:

  // Third: must satisfy _each_ condition of #s#, space-separated
  { type: 'each', 'value': 's', type: 'and', separator: ' ', args: [
    { type: 'or', args: [
      { type: 'contains', args: [ 'title', '#s#' ] },
      { type: 'contains', args: [ 'videosTags.tagName', '#s#' ] },
    ] }
  ] }

Which creates an entry for every space-separated string in conditionsHash['s']. That way, I can make composite queries if needed. At this point, I don't quite understand where to plug that in.

Here is the function...

  function getQueryFromQueryConditions( initialConditions, conditionsHash, allowedFields ){

    // Make up filter.conditions
    function visitQueryConditions( o, fc ){

      // Check o.ifDefined. If the corresponding element in conditionsHash
      // is not defined, won't go there
      if( o.ifDefined ){
        if( ! conditionsHash[ o.ifDefined ] ){ return false; }
      }

      if( o.type === 'and' || o.type === 'or'){
        fc.type = o.type;
        fc.args = [];

        o.args.forEach( function( condition ){

          // If it's 'and' or 'or', check the length of what gets returned
          if( condition.type === 'and' || condition.type === 'or' ){

            // Make up the new condition, visit that one
            var newCondition = {};
            var f = visitQueryConditions( condition, newCondition );

            // Falsy return means "don't continue"
            if( f === false ) return;

            // newCondition is empty: do not add anything to fc
            if(  newCondition.args.length === 0 ){
              return ;
            // Only one condition returned: get rid of logical operator, add the straight condition
            } else if( newCondition.args.length === 1 ){
              var actualCondition = newCondition.args[ 0 ];
              fc.args.push( { type: actualCondition.type, args: actualCondition.args } );
            // Multiple conditions returned: the logical operator makes sense
            } else {
              fc.args.push( newCondition );
            }

          // If it's a leaf
          } else {
            var newCondition = {};
            var f = visitQueryConditions( condition, newCondition );
            if( f !== false ) fc.args.push( newCondition );
          }
        });

      // It's a terminal point
      } else {
        var arg0 = o.args[ 0 ];
        var arg1 = o.args[ 1 ];

        // No arg1: most likely a unary operator, let it live.
        if( typeof( arg1 ) === 'undefined'){
          fc.type = o.type;
          fc.args = [];
          fc.args[ 0 ] = arg0;
        }

        // The second argument has a "but!".
        // If it is in form #something#, it means that it's
        // actually a field in conditionsHash
        var m = ( arg1.match && arg1.match( /^#(.*?)#$/) );
        if( m ) {
          var osf = m[ 1 ];

          // If it's in form #something#, then entry MUST be in allowedFields
          if( ! allowedFields[ osf ] ) throw new Error("Searched for " + arg1 + ", but didn't find corresponding entry in onlineSearchSchema");

          if( conditionsHash[ osf ] ){
            fc.type = o.type;
            fc.args = [];
            fc.args[ 0 ] = arg0;
            fc.args[ 1 ] = conditionsHash[ osf ];
          } else {
            // For leaves, this will tell the callee NOT to add this.
            return false;
          };

        // The second argument is not in form #something#: it means it's a STRAIGHT value
        } else {
          fc.type = o.type;
          fc.args = [];
          fc.args[ 0 ] = arg0;
          fc.args[ 1 ] = arg1;
        }
      }
    }

    var res = {};
    visitQueryConditions( initialConditions, res );


    // visitQueryConditions does a great job avoiding duplication, but
    // top-level duplication needs to be checked here
    if( ( res.type === 'and' || res.type === 'or' )){
      if( res.args.length === 0 ) return {};
      if( res.args.length === 1 ) return res.args[ 0 ];
    }
    return res;
  }



  var initialConditions = {
    type: 'and',
    args: [

      // First of all: video must be published or belonging to logged in user
      { type: 'or', args: [ 
        { type: 'eq', args: [ 'published', true ] },
        { type: 'eq', args: [ 'userId', '#_loggedInUser#' ] },
      ]},

      // Second: filter by userId if passed
      { type: 'eq', args: [ 'userId', '#userId#'] },


      { type: 'or', args: [
        { type: 'contains', args: [ 'title', '#s#' ] },
        { type: 'contains', args: [ 'videosTags.tagName', '#s#' ] },
      ] }

    ]
  };

   var conditionsHash = {
      s: "some",
      userId: "44",
      _loggedInUser: "55"
    };


  var allowedFields = {
    _loggedInUser: true,
    userId: true,
    s: true
  }
  var conditions = getQueryFromQueryConditions( initialConditions, conditionsHash, allowedFields );

  console.log( require('util').inspect( conditions, { depth: 10 }) );

It can run on straight node and it will work (I adapted it from the existing code)

Answer 1

You pretty much NEED better variable names.

You've got a lot of comments in there, describing what everything does. But if you could somehow extract all this to function names and variable names then you wouldn't need all that much comments.

newCondition and actualCondition are good. arg0 and arg1 are okay. f, fc, o, osf, m, res... those don't explain anything. Those short variable names make your code hard to read, to the point where I don't actually want to read it.

The comments don't do your spacing any favor. Here it looks like the comment cuts up the if-chain...

        if(  newCondition.args.length === 0 ){
          return ;
        // Only one condition returned: get rid of logical operator, add the straight condition
        } else if( newCondition.args.length === 1 ){

but it doesn't. The return statement means that there was no way for args of length 0 to go past that if statement anyway.

You'd be better off making it more explicit:

        if( newCondition.args.length === 0 ){
          return;
        }

        // Only one condition returned: get rid of logical operator, add the straight condition
        if( newCondition.args.length === 1 ){

Now it reads like a separate condition.

---

      // If it's a leaf
      } else {
        var newCondition = {};
        var f = visitQueryConditions( condition, newCondition );
        if( f !== false ) fc.args.push( newCondition );
      }

Wrap it in handleLeaf(condition, fc). Your main code should read pretty much as a state machine; "if the next node is of type A, pass it to the type A handler, if it is a type B, send it off to the type B handler", etc. Maybe get used to using different words to refer to a single "condition" and multiple "conditions". Personally, I used Condition and ConditionSet when I needed such a thing in my own code.