5
\$\begingroup\$

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've added comments to hopefully make some more sense,

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 => {
          // Add comment id to post document
          post.comments.push(comment._id);
          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
          const newPostAuthorNotification = new Notification({
            type: 'postAuthorNotification',
            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)
          const newCommentNotification = new Notification({
            type: 'postCommentersNotification',
            createdAt: now,
            subject: {
              id,
              firstName
            },
            postText: savedPost.text,
            postId: savedPost._id,
            recipients: savedPost.commenters
          });

        //Save the commentNotification
        newCommentNotification.save()
          //Find the notification sent before this one that is of the same type
          // and belongs to the same post
          .then(() => Notification
            .find({ type: newCommentNotification.type, postId: savedPost._id })
            .sort({ createdAt: -1 })
            .skip(1)
            .limit(1))
          .then(lastCommentNotification => {
            //IF a previous one exists...
            if (lastCommentNotification[0]) {
              // 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())) {
                const idIndex = lastCommentNotification[0].recipients.indexOf(id);
                if (idIndex > -1) lastCommentNotification[0].recipients = [id];
                else lastCommentNotification[0].recipients = [];
              } else lastCommentNotification[0].recipients = [];

              lastCommentNotification[0].save()
                .then(() => {
                  // if the currentCommenter is not the postAuthor, send a notifiction to the postAuthor
                  if (id.toString() !== savedPost.author.toString()) {
                    newPostAuthorNotification.save()
                      .then(() => {
                        Notification
                          .find({ type: newPostAuthorNotification.type, postId: savedPost._id })
                          .sort({ createdAt: -1 })
                          .skip(1)
                          .limit(1)
                          .then(lastAuthorNotification => {
                            //take the postAuthor out of the previous notification
                            //recipients so they only see then latest notification
                            if (lastAuthorNotification[0]) {
                              lastAuthorNotification[0].recipients = [];
                              lastAuthorNotification[0].save()
                                .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()) {
                newPostAuthorNotification.save()
                  .then(() => {
                    Notification
                      .find({ type: newPostAuthorNotification.type, postId: savedPost._id })
                      .sort({ createdAt: -1 })
                      .skip(1)
                      .limit(1)
                      .then(lastAuthorNotification => {
                        if (lastAuthorNotification[0]) {
                          //if previous exists, show author only latest one
                          lastAuthorNotification[0].recipients = [];
                          lastAuthorNotification[0].save()
                            .then(() => res.send(comment));
                        }
                        return res.send(comment);
                      });
                  });
                  //otherwise, just return the comment
              } else return res.send(comment);
            }
          });
      })
    )
    .catch(next);
  });
\$\endgroup\$
  • \$\begingroup\$ I hope you get some great answers! \$\endgroup\$ – Phrancis Feb 9 '17 at 3:10
2
\$\begingroup\$

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([
            notification.notifyPostAuthorAboutNewComment(savedPost, newComment),
            notification.notifyPostCommentersAboutNewComment(savedPost, newComment)
        ]);
    })
    // ...

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.