1

I'm learning node and I have a piece of code that queries data in the db that i'd like to pass into a callback function. But I'd like to only return unique members.

This means that I have to go through the result of the query, and push the unique members into an array, then pass the array into the cb function.

The problem is, the input is also an array that i build the query off. So there's already a loop there.

I've taken a look at some previous SO threads with promises, await, etc. But I'm having some trouble implementing them with loops like in here.

static getDestination(envName, cb){
    var ans = new Array();
    var promises;
    DataBase.initDB((db) => {
        for (var i = 0; i < envName.length; i++){
            db.collection('Environment').find({'environmentName' : envName[i]}, (err, data) => {
                if (err) {
                    console.log('error getting data in getDestination()');
                    return;
                }
                data.toArray((err, content) => {
                    if (err) {
                        console.log('error converting cursor into array');
                        return;
                    }
                    else {
                        for (const doc of content){
                            if (!ans.includes(doc.destination)){
                                ans.push(doc.destination);
                            }
                            //cb(ans); 
                        }
                    }
                }); 
            });
        }
    })
    cb(ans); 
}

Right now, the callback is just a console.log(ans), and it's printing out an empty array [].

If I call the callback in the innermost loop, it returns the proper members, but it gets called as the loop goes like:

[]
[ 'DEV' ]
[ 'DEV' ]
[ 'DEV', 'QA' ]

I'd like to get only the last line of the output. A thorough explanation would be amazing too!

EDIT: To my understanding, I should be creating an array of promises, and at every iteration of the loop, I should be creating a new promise with the operation as a parameter. Then at the end of everything, I can call

Promise.all(promises).then(cb(ans));

But I'm having trouble understanding which transaction should I create the promises on. Since if I create the promise in the innermost loop, the cb outside of the loop gets called before the first was even created.

EDIT:

exports.initDB = (cb) => {
console.log('Connecting...');
const uri = <mongo connection uri here>;
const client = new MongoClient(uri, { useNewUrlParser: true });
client.connect(err => {
    if (err) {
        console.log('eror connecting to db');
        console.log(err);
        return;
    }
    cb(client.db(process.env.DB_NAME));
});

}

11
  • Possible duplicate of How do I return the response from an asynchronous call? Commented Jul 19, 2019 at 19:40
  • 1
    What library are you using? I searched DataBase.initDB and found this. Is that the code you're using? Commented Jul 19, 2019 at 19:41
  • @jonrsharpe while that's a great generic resource, this question is specific enough to merit its own answer imo. There appears to be no lack of understanding here that the callback is asynchronous, they just need to be shown how to call the callback only under the correct conditions. Commented Jul 19, 2019 at 19:43
  • The title mentions promises, but there are no promises in your code. So what do you expect? If it is an explanation, then the Q&A that jonrsharpe refers to is quite extensive on the topic. Commented Jul 19, 2019 at 19:45
  • 1
    @PatrickRoberts It's just a utility db class that I wrote to initialize connection to MongoDB in the server. I've edited the post to include that piece of code. Commented Jul 19, 2019 at 19:53

1 Answer 1

1

Callbacks

Here's how you'd create your array of promises and use it to aggregate ans then invoke your cb() once. I also suggest using the Node.js callback convention of cb(error, result) rather than cb(result), since there are multiple steps that can error, and the caller of getDestination() should be notified both when the query succeeds or when it fails.

static getDestination (envName, cb) {
  DataBase.initDB(db => {
    const envCollection = db.collection('Environment');
    const promises = envName.map(value => new Promise((resolve, reject) => {
      envCollection.find({ environmentName: value }, (error, data) => {
        if (error) return reject(error);

        data.toArray((error, content) => {
          if (error) reject(error);
          else resolve(content);
        });
      });
    }));

    Promise.all(promises).then(contents => {
      const ans = contents.reduce(
        (ans, content) => content.reduce(
          (ans, doc) => ans.add(doc.destination),
          ans
        ),
        new Set()
      );

      cb(null, [...ans]);
    }).catch(error => {
      cb(error);
    });
  });
}

Notice that each new Promise() is constructed synchronously, and resolve() or reject() is called asynchronously. This is necessary because your database queries do not already return a Promise.

If you use an API that does return promises, you should avoid the explicit promise construction antipattern and chain from the existing promises instead.

I also optimized your aggregation of ans by using a Set instead of an array, so we can actually omit checking for existing values since a set only keeps one of each unique value unlike an array.

Promise chains

If you want your getDestination() function to return a promise instead of accepting a callback, you can rewrite it like this:

static getDestination (envName) {
  return new Promise(resolve => {
    DataBase.initDB(resolve);
  }).then(db => {
    const envCollection = db.collection('Environment');
    const promises = envName.map(value => new Promise((resolve, reject) => {
      envCollection.find({ environmentName: value }, (error, data) => {
        if (error) return reject(error);

        data.toArray((error, content) => {
          if (error) reject(error);
          else resolve(content);
        });
      });
    }));

    return Promise.all(promises);
  }).then(contents => {
    const ans = contents.reduce(
      (ans, content) => content.reduce(
        (ans, doc) => ans.add(doc.destination),
        ans
      ),
      new Set()
    );

    return [...ans];
  });
}

The key differences are:

  • wrap DataBase.initDB() in another Promise because the result depends on db
  • return the promise chain from getDestination()
  • return [...ans] from .then() instead of passing it to a callback
  • remove .catch() and let the caller handle any errors

Notice that because we return Promises.all(promises) from a .then(), we can get contents from the outer promise instead of chaining to the inner Promise.all(). This is one of the major benefits that promises offer to escape from callback hell.

Async / Await

Lastly, if you want to use async / await instead of .then(), you can rewrite it once more like this:

static async getDestination (envName) {
  const db = await new Promise(resolve => {
    DataBase.initDB(resolve);
  });

  const envCollection = db.collection('Environment');
  const promises = envName.map(value => new Promise((resolve, reject) => {
    envCollection.find({ environmentName: value }, (error, data) => {
      if (error) return reject(error);

      data.toArray((error, content) => {
        if (error) reject(error);
        else resolve(content);
      });
    });
  }));

  const contents = await Promise.all(promises);
  const ans = contents.reduce(
    (ans, content) => content.reduce(
      (ans, doc) => ans.add(doc.destination),
      ans
    ),
    new Set()
  );

  return [...ans];
}

The key difference here is that each

return somePromise.then(someVariable => { ... });

is replaced by

const someVariable = await somePromise;
...

Modularization

P.S. if you want to remove the constructed promises from your getDestination() function, you can refactor your code into two helper functions that can be used like this:

static getCollection (collectionName) {
  return new Promise(resolve => {
    DataBase.initDB(resolve);
  }).then(
    db => db.collection(collectionName)
  );
}

static getDocuments (collection, ...args) {
  return new Promise((resolve, reject) => {
    collection.find(...args, (error, data) => {
      if (error) return reject(error);

      data.toArray((error, content) => {
        if (error) reject(error);
        else resolve(content);
      });
    });
  });
}

static async getDestination (envName) {
  const envCollection = await this.getCollection('Environment');
  const promises = envName.map(
    environmentName => this.getDocuments(
      envCollection,
      { environmentName }
    )
  );

  const contents = await Promise.all(promises);
  const ans = contents.reduce(
    (ans, content) => content.reduce(
      (ans, doc) => ans.add(doc.destination),
      ans
    ),
    new Set()
  );

  return [...ans];
}

It may be slightly more code than before, but now you have two re-usable functions that can be called from other kinds of queries besides getDestination().

Sign up to request clarification or add additional context in comments.

4 Comments

Thank you for an amazingly detailed explanation. I'm trying to understand each one right now, For the cb one, just to make sure I understand what's going on there: 1) Go through envName here and create a new promise for each find query 2) The promise resolves with the query result converted into an array 3) If all the promises are resolved 4) Create a set (ans) from the array of resolves (contents)
So, I just ran it and got: TypeError: Cannot read property 'add' of undefined at content.reduce. ans is a new Set() correct? It's just declared in the outermost reduce().
@Lavzh I omitted the { ... } from the callbacks of reduce() on purpose. If you added them in, you need to return ... in the body. At least that's what it seems like you did.
Thanks so much Patrick, the cb one works perfectly, though i used for loops since I understand it a little better, instead of the reduce. I'm still going through the other solutions, especially the modularized one!

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.