Promises with Express/Mongoose to send comment notifications

I'm building the backend for a mobile app project.

Below is my createComment controller.

The challenge for me is that I want to send notifications out to the postAuthor and other commenters.

If a user is either a postAuthor or commenter, and 20 users make a comment, I do not want there to be 20 notifications in their notification feed.

I only want the most recent one to be shown. So to do this, I find the last notification of the same type and id, and then alter the recipient array.

When a user GETS their notifications, then it will grab the notifications that they are a recipient of.

I have around 10 mocha tests running confirming that it works. I've been working on this for the past two days and it's taken me around 20 hours to get all the tests passing.

I'm a beginner, this is my first real project I've decided to build.

Having said that, I'm very aware that the code below is disgusting. I've tried a long time now trying to make it look better but I'm having little luck.

I'd really appreciate some feedback/suggestions on how I can improve up please, thank you:

app.post('/api/comments/:id', (req, res, next) => {
const commentProps = req.body;

//Post that the comment belongs to
const postId = req.params.id;

const now = new Date().getTime();

const { author, text } = commentProps;
const { id, firstName, university } = author;

// Building the comment
const newComment = {
author: {
id,
firstName,
university
},
text,
postId,
createdAt: now,
};

// Create comment
Comment.create(newComment)
.then(comment => Post.findById(postId)
.then(post => {
if (post.commenters.indexOf(id) === -1) {
post.commenters.push(id);
}
return post.save();
})
.then(savedPost => {
//if postAuthor is a commenter, take them out of comment array
const authorIndex = savedPost.commenters.indexOf(savedPost.author);
if (authorIndex > -1) savedPost.commenters.splice(authorIndex, 1);

//take commentAuthor out of comment array
const commentAuthorIndex = savedPost.commenters.indexOf(id);
if (commentAuthorIndex > -1) savedPost.commenters.splice(commentAuthorIndex, 1);

//This is the notification for the postAuthor
createdAt: now,
subject: {
id,
firstName
},
postText: savedPost.text,
postId: savedPost._id,
recipients: savedPost.author
});

//This is the notification for the commenters(minus the current commentAuthor)
createdAt: now,
subject: {
id,
firstName
},
postText: savedPost.text,
postId: savedPost._id,
recipients: savedPost.commenters
});

//Find the notification sent before this one that is of the same type
// and belongs to the same post
.find({ type: newCommentNotification.type, postId: savedPost._id })
.sort({ createdAt: -1 })
.skip(1)
.limit(1))
//IF a previous one exists...
// modify the recipients so that commenters won't see it, unless they're the currentCommenter
if ((id.toString() !== lastCommentNotification[0].subject.id.toString()) && (id.toString() !== savedPost.author.toString())) {
if (idIndex > -1) lastCommentNotification[0].recipients = [id];

.then(() => {
// if the currentCommenter is not the postAuthor, send a notifiction to the postAuthor
if (id.toString() !== savedPost.author.toString()) {
.then(() => {
.find({ type: newPostAuthorNotification.type, postId: savedPost._id })
.sort({ createdAt: -1 })
.skip(1)
.limit(1)
//take the postAuthor out of the previous notification
//recipients so they only see then latest notification
.then(() => res.send(comment));
}
return res.send(comment);
});
});
} else return res.send(comment);
});
} else {
//if a previous comment doesn't exist and the current commenter
//is not the postAuthor, send a postAuthor notification
if (id.toString() !== savedPost.author.toString()) {
.then(() => {
.find({ type: newPostAuthorNotification.type, postId: savedPost._id })
.sort({ createdAt: -1 })
.skip(1)
.limit(1)
//if previous exists, show author only latest one
.then(() => res.send(comment));
}
return res.send(comment);
});
});
//otherwise, just return the comment
} else return res.send(comment);
}
});
})
)
.catch(next);
});

• I hope you get some great answers! – Phrancis Feb 9 '17 at 3:10

I don't quite understand why you need to save notifications in the first place. It seems you could just find them on the fly when the user GET's them.

That said, here's a few comments:

1: It would be nice if you had split your app into smaller building blocks. Then you could write something like this:

Comment.create(newComment)
// ...
.then(savedPost => {
return Promise.all([
]);
})
// ...


And of course you could split those two functions up there into even smaller pieces.

2: I would do something about the code below (and similar code):

//if postAuthor is a commenter, take them out of comment array
const authorIndex = savedPost.commenters.indexOf(savedPost.author);
if (authorIndex > -1) savedPost.commenters.splice(authorIndex, 1);

//take commentAuthor out of comment array
const commentAuthorIndex = savedPost.commenters.indexOf(id);
if (commentAuthorIndex > -1) savedPost.commenters.splice(commentAuthorIndex, 1);


Notice how you have to have comments to explain what it does. Also, notice how you're changing the savedPost.commenters array, when it's actually not the commenters you are after, it's the recipients.

Instead, you could create a few well named functions to explain yourself, and put the results into a new array.

let isAuthorOfPost = commenterId => commenterId === savedPost.author;
let isAuthorOfCurrentComment = commenterId => commenterId === newComment.author.id;

let recipients = savedPost.commenters
.filter(commenter => !isAuthorOfPost(commenter))
.filter(commenter => !isAuthorOfCurrentComment(commenter));


This code is not efficient at all, but that's less important than readability.

3: Try to avoid nested promises. It becomes harder to read, and harder to keep track of errors. There are a bunch of places where you're missing a catch statement, and rejections are left unhandled. This would have been a non-issue if it weren't for the nesting. Also see point 1 about smaller building blocks.