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);
 }
 });
}