0

I have a function making a callback to other functions in the class in linear fashion.

async runTask() {
    debug('runTask');
    debug(this.taskData);

    try {
      // Short circuit for testing
      if (process.env.TASKTIMER) {
        return setTimeout(() => {
          this.taskComplete();
        }, process.env.TASKTIMER);
      }

      // Setup project folder & Git Clone
      await this.pullPackageWait();

      // Create input and output directory
      await this.mkIODirWait();

      // Store task data as json
      await this.writeTaskDataJSONWait();

      // Download Inputs From Amazon
      await this.downloadInputsWait();

      // Link Input files
      await this.linkInputsWait();

      // Download Resources From Amazon
      await this.downloadResourcesWait();

      // Relax permission on tmp directory
      await this.chmodTaskDir();

      // Destroys the job after timeout
      await this.jobTimeout();

      // Determine Handler
      console.log(this.taskData.handler);
      await this.initHandler();
      await this._handler.startJob();
    } catch (err) {
      this.logger.error(err);
      return this.taskError();
    }

I'm trying to convert the function calls into an array of functions following the this answer. After refactoring my functions looks as follows:

 async runTask() {
    debug('runTask');
    debug(this.taskData);

    try {
      // Short circuit for testing
      if (process.env.TASKTIMER) {
        return setTimeout(() => {
          this.taskComplete();
        }, process.env.TASKTIMER);
      }

      let currentFunction = 0;
      const functionsToRun = [
        this.pullPackageWait,
        this.mkIODirWait,
        this.writeTaskDataJSONWait,
        this.downloadInputsWait,
        this.linkInputsWait,
        this.downloadResourcesWait,
        this.chmodTaskDir,
        this.jobTimeout,
      ];

      debug('Before The Loop');
      for (currentFunction; currentFunction < functionsToRun.length; currentFunction++) {
        debug('In The Loop');
        await functionsToRun[currentFunction]();
        console.log('Writing to the file');
        await this.writeState();
      }

      // Determine Handler
      console.log(this.taskData.handler);
      await this.initHandler();
      await this._handler.startJob();
    } catch (err) {
      this.logger.error(err);
      return this.taskError();
    }

With the original code the entire program runs normally, but after conversion it seems to break at the first callback. Did I make a mistake somewhere in conversion?

Yury Stanev
  • 419
  • 1
  • 5
  • 16
  • functionsToRun elements can be strings and called `this[currentFunction]()`, seems like wasted memory to me – Lawrence Cherone Sep 05 '19 at 15:49
  • You probably have issues with `this` binding. Either use Lawrence's idea and store the function names as strings or store them as `this.pullPackageWait.bind(this) ...` etc – slebetman Sep 05 '19 at 15:54
  • I've had them as string before, but that seems to be messing with types, maybe it was because of `for of`. I'll give it a try and see what happens. – Yury Stanev Sep 05 '19 at 15:55
  • nitpick, replace `console.log` with `debug`, your thank me later (code review) ;p – Lawrence Cherone Sep 05 '19 at 15:58
  • @LawrenceCherone Arguably storing a function pointer (4 or 8 bytes per function depending on 32 or 64 bit interpreter) uses less memory than the function names as strings (between 10-20 bytes per function) – slebetman Sep 05 '19 at 16:17

1 Answers1

1

The most likely source of error in the refactored code is incorrect this binding. Note that the following:

this.foo();

is not the same as:

let foo = this.foo;
foo();

When calling this.foo() the this value inside foo() will be the same this as this.foo. When calling foo() the this value inside foo() will either be undefined or the global object, depending on weather you have strict mode on or not.

For a complete explanation of how this works refer to my answer to this other question: How does the "this" keyword in Javascript act within an object literal?


TLDR

The solution is to either store the function names as strings:

const functionsToRun = [
    'pullPackageWait',
    'mkIODirWait',
    'writeTaskDataJSONWait',
    'downloadInputsWait',
    'linkInputsWait',
    'downloadResourcesWait',
    'chmodTaskDir',
    'jobTimeout',
];

Then call each function as:

await this[functionsToRun[index]]();

Or .bind() the functions:

const functionsToRun = [
    this.pullPackageWait.bind(this),
    this.mkIODirWait.bind(this),
    this.writeTaskDataJSONWait.bind(this),
    this.downloadInputsWait.bind(this),
    this.linkInputsWait.bind(this),
    this.downloadResourcesWait.bind(this),
    this.chmodTaskDir.bind(this),
    this.jobTimeout.bind(this),
];

and call them the way you're currently doing:

await functionsToRun[currentFunction]();
slebetman
  • 109,858
  • 19
  • 140
  • 171