JS ES6 Node/Express Mongo code that processes a CSV file

Question

I'm a java dev that has started working with Node/Express and mongo. I'm looking to see how I can improve my code, especially in terms of the async jazz as that is still a bit new to me (and I feel like my async usage is a mess). Here is my function, please let me know what I can do to improve, implement best practices, or clean up my code. Thanks so much!

/*

This function takes extracts base64 encoded CSV data and parses a list of 
students from it.  Each record is checked to
ensure that it doesn't already exist in the DB. If the student doesn't exist 
it is added.  The method returns the number of
entries added to the database and a list of any processing errors.

 */
exports.uploadStudents = function (req, res, next) {

    const buildStudent = function (line) {
        if (line && line.length >= 5) {
            const studentNumber = line[0];
            const first = line[1];
            const last = line[2];
            const classOf = line[3];
            const location = line[4];

            if (textUtils.isNullOrBlank(studentNumber)) {
                throw new Error(`Invalid student number: ${studentNumber}`);
            }

            // the header row in our template prefixes the description with ** so we can easily filter those out.
            if (studentNumber.startsWith("**")) {
                throw new Error(`Row rejected: header row`);
            }

            if (textUtils.isNullOrBlank(first)) {
                throw new Error(`Invalid first name: ${first}`);
            }

            if (textUtils.isNullOrBlank(last)) {
                throw new Error(`Invalid last name: ${last}`);
            }

            // mongoose backed model
            var student = new Student();
            student.studentNumber = studentNumber;
            student.firstName = first;
            student.lastName = last;
            student.classOf = classOf;
            student.location = location;

            return student;
        } else {
            throw new Error(`Invalid number of columns ${line.length}`);
        }
    };

    /*
    check with the DB to see if the student is already present and return a promise of a wrapper object or throw an error
     */
    const studentExists = async function (student) {
        if (student) {
            const list = await Student.find({'studentNumber': student.studentNumber}).exec();
            if (list.length === 0) {
                return {exists: false, student: student};
            } else {
                return {exists: true, student: student};
            }
        } else {
            throw new Error('Student not found' + JSON.stringify(student.studentNumber));
        }
    };

    var buffer = Buffer.from(req.body.data, 'base64');
    var errors = [];

    Papa.parse(buffer.toString(), {
        skipEmptyLines: true,
        error: function (err) {
            debug("Error: " + err);
            return next(err);
        },
        complete: function (results, file) {

            // process linearly
            const students = results.data.map(row => {
                try {
                    return buildStudent(row);
                } catch (e) {
                    errors.push(e.message);
                }
                return null;
            }).filter(s => s != null);


            // lets see what students we already have in the DB
            const allPromises = students.map(s => studentExists(s));
            // we want to resolve all the promises
            Promise.all(allPromises).then(async listOfResults => {  // make async to use the poor man's finally
                const missingStudents = listOfResults.filter(r => !r.exists).map(r => r.student);

                // results will hold a promises for each student processed that will contain 1 if successful and or 0 if the
                // process failed so we can sum them up to get a total number of students added with this import
                const results = missingStudents.map(async missingStudent => {
                    try {
                        // could use insertMany, but I want details on each if there is a failure so slow and more details
                        // is better than fast and no details
                        const result = await missingStudent.save();
                        if (result) {
                            return 1;
                        }
                    } catch (e) {
                        debug(`Error saving student: ${e.message}`);
                        errors.push(e.message);
                    }
                    return 0;
                });

                return (await Promise.all( results )).reduce((a, b) => a + b, 0);
            }).then(countOfStudentsAdded => {
                // why no official finally :( ... use the poor mans finally
                const ret = {'added': countOfStudentsAdded, 'errors': errors};
                return res.json(ret);
            }).catch(e => {
                return next(e);
            });
        }
    });
};

Answer 1

Callouts

• Utilizing es6 to it's fullest (deconstructing obj/arrays, arrow functions)
• Linting (redefining keys ex {errors: errors}, quoting key)
  • Installing eslint will call this out
• Simplifying returns

---

exports.uploadStudents = (req, res, next) => {
  const buildStudent = (line) => {
    if (line && line.length >= 5) {
      /*
      * const studentNumber = line[0];
      * const first = line[1]; //if you call this firstName you wont have to rename it when you return new student
      * const last = line[2]; //if you call this lastName you wont have to rename it when you return new student
      * const classOf = line[3];
      * const location = line[4];
      */
      //More easily defined like so
      const [studentNumber, first, last, classOf, location] = line;
      if (textUtils.isNullOrBlank(studentNumber)) {
        throw new Error(`Invalid student number: ${studentNumber}`);
      }

      // the header row in our template prefixes the description with ** so we can easily filter those out.
      if (studentNumber.startsWith("**")) {
        throw new Error(`Row rejected: header row`);
      }

      if (textUtils.isNullOrBlank(first)) {
        throw new Error(`Invalid first name: ${first}`);
      }

      if (textUtils.isNullOrBlank(last)) {
        throw new Error(`Invalid last name: ${last}`);
      }

      /*
      * // mongoose backed model
      * var student = new Student();
      * student.studentNumber = studentNumber;
      * student.firstName = first;
      * student.lastName = last;
      * student.classOf = classOf;
      * student.location = location;

      * return student;
      */
      // Yous should be able to shorten this up...

      return new Student({
        studentNumber,
        firstName: first, //renaming first to firstName above would reduce the need to for renaming the key
        lastName: last, //renaming last to lastName above would reduce the need to for renaming the key
        classOf,
        location,
      });
    } else {
      throw new Error(`Invalid number of columns ${line.length}`);
    }
  };

  /*
  check with the DB to see if the student is already present and return a promise of a wrapper object or throw an error
  */
  const studentExists = async (student) => {
    if (student) {
      const list = await Student.find({ studentNumber: student.studentNumber }).exec();
      /*
      * if (list.length === 0) {
      *     return {exists: false, student: student};
      * } else {
      *     return {exists: true, student: student};
      * }
      */
      // cleaner to write
      return {
        exists: list.length === 0, //this will set your boolean
        student // no need to repeat key name in es6
      };
    } else {
      throw new Error(`Student not found ${JSON.stringify(student.studentNumber)}`);
    }
  };

  const buffer = Buffer.from(req.body.data, 'base64');
  const errors = [];

  Papa.parse(buffer.toString(), {
    skipEmptyLines: true,
    /* error: function (err) { */
    //shorten up with arrow function
    error: (err) => {
      debug("Error: " + err);
      return next(err);
    },
    /* complete: function (results, file) { */
    //shorten up with arrow function
    complete: (results, file) => {
      // process linearly
      const students = results.data.map(row => {
        try {
          return buildStudent(row);
        } catch (e) {
          errors.push(e.message);
          return null;
        }
        //this is only hit in the catch, I would move it up for simplicity of reading
        /* return null; */
      }).filter(s => !s);

      // lets see what students we already have in the DB
      const allPromises = students.map(studentExists);
      // we want to resolve all the promises
      Promise.all(allPromises).then(async listOfResults => {  // make async to use the poor man's finally
      const missingStudents = listOfResults.filter(({ exists }) => !exists).map(({ student }) => student);

      // results will hold a promises for each student processed that will contain 1 if successful and or 0 if the
      // process failed so we can sum them up to get a total number of students added with this import
      const results = missingStudents.map(async (missingStudent) => {
        try {
          // could use insertMany, but I want details on each if there is a failure so slow and more details
          // is better than fast and no details
          /*
          * const result = await missingStudent.save();
          * if (result) {
          */
          // You are not using result more than once, don't bother allocating space for it
          if(await missingStudent.save()) {
          return 1;
          }
          /*
          * } catch (e) {
          *    debug(`Error saving student: ${e.message}`);
          */
          //You can deconstruct e if you prefer
        } catch ({ message }) {
          debug(`Error saving student: ${message}`);
          errors.push(message);
          return 0;
        }
        //again this return is only hit in the catch, i would move it up for readability
        /* return 0; */
      });
      return (await Promise.all( results )).reduce((a, b) => a + b, 0);
      }).then(countOfStudentsAdded => {
        // why no official finally :( ... use the poor mans finally
        /*
        * const ret = {'added': countOfStudentsAdded, 'errors': errors};
        * return res.json(ret);
        */
        // dont define vars that aren't reused (`ret`) don't redefine keys (`errors`), don't quote keys
        return res.json({
          added: countOfStudentsAdded,
          errors,
        });
      }).catch(next);
    }
  });
}