Just spacing changes here to make it consistent with everythign else, idk why it was totally different here.

This is my new service, notice my file naming convention of survey.service.js. It's all lowercase, and has a .type naming convention, that way you know what type of object your dealing with. I kept it in the services folder for now, but as I mentioned you should group by feature, it's just not worth doing at this point on this small of a project.

Since this was a bunch of logic and wrote to the database, it needed moved into a service, so I did that.

It's all replaced by me calling my service method and sending the result along.

I also added a helper method to my service to handle this confusing logic. Putting this code directly in an object definition is gross. Instead I made a nicely named function in my service and I call it here. Because it's nicely named, you know what it does just by looking, getRecipientListFromEmailString. It also keeps more business logic in the service layer. Really most of this could be moved to the service, but whatever, this is fine for now.

Passing a function call into a function is kind of ugly, I'd rather have one more line of code and have it be easier to understand. So I create a new variable surveyContent and store the filled survey there. Then I pass it into the Mailer constructor.

This was the confusing argument destructor I was talking about. This constructor, as we saw just above, takes a survey as it's first argument. However, you wouldnt' know that just by looking at it due to the argument deconstructions.

I think it's much easier to ready by simply saying a survey object is passed in.

Since we didn't deconstruct the survey when it was passed in, we have to access the properties of the survey, so I add the survey.. This to me is just much easier to understand than before.

For simple arrow function callbacks, you can make it a single line and omit the curly brace and return keyword, which I find easier to read.

The function that calls this method also uses async/await...which makes no sense. Using it here means this simply returns the result of the API call, you don't await whatever this api call returns.

Since the function that calls it calls other async functions, it makes sense to simply remove it here.

The calling function looks like this:

try {
      await mailer.send();  // This makes no sense with the old code, since .send() will simply return a response, there's nothing to "await" because the .send() call already waited to return the object.
      await survey.save();
      req.user.credits -= 1;
      const user = await req.user.save();

      res.send(user);
    } catch (err) {
      res.status(422).send(err);
    }

Again, simple code so why not remove the curly braces and one-line it.

So, here's my big service method that does some work on the data sent in and then updates the database. The old code had many problems besides just being in the router.

The biggest was that even though it was an async operation (it wrote to the database), it did not use async await. This meant that if there was an error writing to the database, you would never know it. It would simply return instantly while asyncrhounously the database would be written to. I fix that by actually using async/await here, and wrapping it in a try catch block.

Notice I also do not use lodash at all, as it is obsolete and just another thing to learn that you don't need. It was also being used incorrectly, as the uniqBy function only takes one argument...so there was actually a major functional bug in the code.

Here I chain regular old Javascript array methods. I take the data passed in and then I run some operations on it. This is the same as what was being done with lodash, except it uses native javascript, and actually works as it's supposed to.

Also notice that I broke each part of the functionality into it's own named function. This makes the chain of events much simpler to ready. You can see that first I buildSurveyDataFromBody, then I filterOutNullAndUndefined , and finally filterByUniqueEmailAndSurveyId.

The functionality for each of those is stored in it's own nicely named function. This helps readability, and keeps your logic separted nicely. Remember you want functions to try to serve a single purpose. Break things down into their smallest logical pieces and name them nicely.

If we weren't using async/await for this, I would just chain a .forEach onto my javascript array chain, which would look nice:

data.map(this.buildSurveyDataFromBody)
  .filter(this.filterOutNullAndUndefined)
  .filter(this.filterByUniqueEmailAndSurveyId)
  .forEach(this.writeSurveysToDatabase);

But forEach doesn't work with async/await, so I had to use a for/of loop instead. This mean the loop waits until the database write is complete before continuing, and then this function won't return until all writes are complete.

Rather than returning nothing like the old method, I return this success data just for a way to verify it worked.

And here if there was an error, I return that error. You wouldn't want to do this in a real production app, as it could expose details about your code or database that you wouldn't want the public to know, but for this example it's fine, and will actually let you know theres an error, unlike the original code which would fail silently.

This is that helper function for the recipient formatting, pretty basic, make an array from a comma separated string and then turn those strings into object with email as the key and the string as the value.

Just noticed an error from when I moved code around. I call the passed in object bodyItem, but then later call it surveyData. I forgot to fix this after moving it into it's own service. They should be the same thing.

This is called by the map function (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map). Map just runs on an array and returns a new array based on the function passed in. So here we had an array of objects with emails and urls, and we turn it into an array of objects with emails, surveyIds, and choices.

That previous map function passes that array of data to this filter callback, which removes any null or undefined items. This should be impossible since the .map function above only adds items that match the URL, but since it was in the original code and I didn't actually test to be sure...I left it in. This can probably be removed though.

This part replaces lodash, it's another filter method here. The lodash method was actually broken, as he tried to make it unique by both email and survey ID, but if you check the lodash docuemtation, uniqBy only takes a single string to be unique by. So...his old code was only unique by email, not survey ID. Meaning...it was broken.

This works properly and filters down to a unique list by email and survey ID, by creating a new array with a key string of email + survey ID, and testing if the index of the current email + id is the same as the current index.