ES6 cron module

Question

I just created my first ES6 module in Node, and first time using promises and I was hoping to get some feedback on it. I am the only developer at my current company, so I cannot get feedback here. I just want to make sure whoever comes behind me in the future can have a clean code base to work in.

'use strict';
var fs = require('fs');
var nodeSchedule = require('node-schedule');

class CronJob {
    constructor(filePath, time, rule) {
        this.filePath = filePath;
        this.maxPdfAge = time || 86400000;
        this.currentDate = Date.parse(new Date());
        this.scheduleRule = rule || this.getDefaultRule();
    }
    schedule(callback) {
        nodeSchedule.scheduleJob(this.scheduleRule, err => {
            if (err) return callback(err);

            this.scanFolder((err, info) => {
                if (err) return callback(err, null);
                return callback(null, info);
            });
        });

    }
    scanFolder(callback) {
        scanDirectory(this.filePath)
            .then(files => {
                this.findOldPdfs(this.filePath, files, (err, info) => {
                    if (err) return callback(err, null);
                    return callback(null, info);
                });
            }).catch(err => {
                callback(err, null);
            });
    }
    findOldPdfs(filePath, pdfs, callback) {
        var pdfDates = getPdfCreateDates(filePath, pdfs);
        Promise.all(pdfDates)
            .then(pdfs => {
                this.deletePdfs(pdfs, (err, info) => {
                    if (err) return callback(err, null);
                    return callback(null, info);
                });
            })
            .catch(err => {
                callback(err, null);
            });
    }
    deletePdfs(pdfObjects, callback) {
        var pdfsToDelete = getPdfsToDelete(pdfObjects, this.currentDate, this.maxPdfAge);
        Promise.all(pdfsToDelete)
            .then(successMsgs => {
                callback(null, successMsgs);
            }).catch(err => {
                callback(err, null);
            });
    }

    getDefaultRule() {
        var rule = new nodeSchedule.RecurrenceRule();
        rule.dayOfWeek = [0, new nodeSchedule.Range(0, 6)];
        rule.hour = 23;
        rule.minute = 59;
        rule.second = 0;

        return rule;
    }
}


/********************************************/
// Promise function wrappers
/********************************************/

function scanDirectory(filePath) {
    return new Promise((resolve, reject) => {
        fs.readdir(filePath, (err, data) => {
            if (err) {
                reject(err);
            } else {
                resolve(data);
            }
        });
    });
}

function getPdfCreateDates(filePath, files) {
    var arrayOfPromises = [];
    files.forEach(file => {
        var pdfFile = filePath + '/' + file;
        var statLookupPromise = new Promise((resolve, reject) => {
            fs.stat(pdfFile, (err, info) => {
                if (err) {
                    reject(err);
                } else {
                    resolve({ filePath: pdfFile, dateTime: info.mtime });
                }
            });
        });
        arrayOfPromises.push(statLookupPromise);
    });
    return arrayOfPromises;
}

function getPdfsToDelete(pdfs, currentDate, maxPdfAge) {
    var arrayOfPdfPromises = [];
    pdfs.forEach(pdf => {
        if (currentDate - maxPdfAge > pdf.dateTime) {
            var deletePdfPromise = new Promise((resolve, reject) => {
                fs.unlink(pdf.filePath, (err) => {
                    if (err) {
                        reject(err);
                    } else {
                        resolve(pdf.filePath + ' was deleted');
                    }
                });
            });
            arrayOfPdfPromises.push(deletePdfPromise);
        }
    });
    return arrayOfPdfPromises;
}

module.exports = CronJob;

Answer 1

1. Watch your names:
   • CronJob - seems too general to me. It's just to do with deleting old pdfs right?
   • findOldPdfs - this one calls deletePdfs, so a better name might be deleteOldPdfs
   • scanFolder - this one also ends up deleting pdfs, so maybe deleteOldFilesInFolder
   • schedule - same here, ends up deleting pdfs. If your class name was something like OldPdfRemover/PdfCleaner/etc it would make more sense.

Think about how this will be called:

cronJob->schedule();

vs

pdfCleaner->schedule();

2. Consider splitting this into two classes. One to do with cron jobs, and one to do with deleting pdfs.
3. Consider injecting currentDate. The code is hard to test when there is a variable in there which is changing all the time.

4. Why keep callback interfaces to your methods? You can return promises directly, and get rid of all of these:

   .then(successMsgs => { callback(null, successMsgs); }).catch(err => { callback(err, null); });

5. Consider naming your "private" methods with an underscore (ex. _deletePdfs) to make it clear that these are not to be called from the outside

6. Consider using a more general promise adapter for callbacks. You don't need to write this code more than once. Either write your own, or check out denodify. The words "new Promise" should be a rare sight in your code base.