Two similar MongoDB-querying functions to find distinct and non-distinct documents

Question

I'd like to remove some of the repeated code in my mongo data access layer. The code is all the same except for these key areas:

1. Function signature parameters differ between collectionFind and collectionDistinct though they both have a callback cb.
2. The line below // HERE IS THE LINE THAT VARIES

I suspect I can use a closure to pass in cb to the (innerErr, result) callback but that only solves part of the problem so was curious about other ideas.

 collectionFind: function collectionFind(collectionName, query, fields, options, cb) {
 var url = getUrl();
 if (!url) {
 logger.error('No mongodb connection set');
 } else {
 getConnection(url, (err, db) => {
 if (err) {
 logger.error(`Cannot connect to mongo: ${err}`);
 clearConnection();
 } else {
 // HERE IS THE LINE THAT VARIES
 find(db, collectionName, query, fields, options, (innerErr, result) => {
 if (innerErr) {
 clearConnection();
 cb(innerErr);
 } else {
 cb(null, result);
 }
 });
 }
 });
 }
 },
 collectionDistinct: function collectionDistinct(collectionName, field, filter, cb) {
 let url = getUrl();
 if (!url) {
 logger.error('No mongodb connection set');
 } else {
 getConnection(url, (err, db) => {
 if (err) {
 logger.error(`Cannot connect to mongo: ${err}`);
 } else {
 // HERE IS THE LINE THAT VARIES
 distinctDocuments(db, collectionName, field, filter, (innerErr, result) => {
 if (innerErr) {
 clearConnection();
 cb(innerErr);
 } else {
 cb(null, result);
 }
 });
 }
 });
 }
 }

Answer 1

You could return early on errors. This way, you don't nest as much.

I'm not sure if getConnection, find and distinctDocuments are custom functions or library functions based on the example. But if you are able to update their implementation or swap it out for another library, use Promises. That way you avoid callback hell.

Also, I really hate functions that do too much. Some developers tend to just put multiple similar functionalities in one function and distinguish them via an argument. But when more functionality is added or when the functionalities diverge, the implementation becomes monolithic and hard to refactor. So I suggest that until a third or fourth function with the same flow comes in, I would keep it as is.

If the implementation used promises, it would be as easy as:

collectionFind: function collectionFind(collectionName, query, fields, options) {
 const url = getUrl();
 if (!url) return logger.error('No mongodb connection set');
 return getConnection(url)
 .then(db => find(db, collectionName, query, fields, options))
 .catch(err => {
 logger.error(err);
 clearConnection();
 throw err; // rethrow
 });
}
collectionDistinct: function collectionDistinct(collectionName, field, filter) {
 const url = getUrl();
 if(!url) return logger.error('No mongodb connection set');
 return getConnection(url)
 .then(db => distinctDocuments(db, collectionName, field, filter))
 .catch(err => {
 logger.error(err);
 clearConnection();
 throw err; // rethrow
 });
}
// Usage
ns.collectionFind(...).then(result => {
}, err => {
});
ns.collectionDistinct(...).then(result => {
}, err => {
});

• ss2k: Thanks. Yes, I can use promises and this is much nicer. It saves on lines of code and increases readability. I do have have wonder if there's any way to centralize the logic flow itself and pass in as a function find or distinctDocuments. Their differing signatures makes that challenging. I put two examples of functions here too but in my data access layer I have like a dozen just like this with the one line difference. If I want to do something more in the catch for instance I'll be changing it in a dozen places.