ES6 cron module

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, suc

• 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();

• Consider splitting this into two classes. One to do with cron jobs, and one to do with deleting pdfs.

• Consider injecting currentDate. The code is hard to test when there is a variable in there which is changing all the time.

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

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

• 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.

StackExchange Code Review Q#159450, answer score: 2