how to avoid the explicit-construction anti-pattern and still keep indentation/callbacks low

Question

Lets say we have a function, that calls a fair amount of async functions like so:

downloadAndAssemble = () => 
  new Promise(async (resolve, reject) => {

    if (!(await checkAvailableDiskSpace())) {
      resolve(false);
      return;
    }

    try {
      // Download all
      let files  = await this.downloadAll();

      // Assemble File from downloaded snippets.
      const assembledFile = await buildFile(files).catch(error => reject(error));

      // Save assembled file.
      resolve(true);
    } catch (err) {
      reject(err);
    } finally {
      const dirExists = await fs.exists(tmpFolderPath);
      if (dirExists) await fs.unlink(tmpFolderPath);
    }
  });

The first problem I see is new Promise(async (resolve, reject) => { which is an anti pattern according to this SO article.

The general Idea I got from that article is to reuse Promises if available, and not create new ones.

If I follow the suggestions in this SO answer I should use .then and .catch in the logic flow of the function to utilize the existing Promises.

But this would result in more indentations (i.e. one per promise used), which I thought Promises should help eliminate.

As you can see from .catch(error => reject(error)) the code is not very consistent with handling errors thrown by Promises contained within it.

Just to make sure, do you really need the `fs.exists` and `fs.unlink` to run whether an error occurs in the lower section or not with that `finally`? — CertainPerformance, May 29 '19 at 10:04
Make `downloadAndAssemble` an `async` function: `downloadAndAssemble = async () => ...` — Jack Bashford, May 29 '19 at 10:06

score 1 · Accepted Answer · answered May 29 '19 at 10:11

  downloadAndAssemble = async () =>  {
    if (!(await checkAvailableDiskSpace())) {
      return false;
    }

     try {
       // Download all
       let files  = await this.downloadAll();

       // Assemble File from downloaded snippets.
       const assembledFile = await buildFile(files);

      // Save assembled file.
      return true;
    } finally {
       const dirExists = await fs.exists(tmpFolderPath);
       if (dirExists) await fs.unlink(tmpFolderPath);
    }
};

If you call an async function, it will implicitly create a promise under the hood for you, that resolves if you return and rejects if you throw, so there is no need to create and manage a promise "manually".

.catch(error => reject(error)) makes little sense as awaiting a promise automatically lets the errors bubble up. In your code this bypasses the try { ... } catch { ... } which is probably not wanted.

The same applies to } catch (err) { reject(err); }, await is all you need.

That cleared a lot up, thanks! Just to make sure: This works if only Promises are involved. As soon as we use some function utilizing callbacks, we will have to convert that callback-structure into a promise first so we can then use async/await exclusively, correct? — David Schumann, May 29 '19 at 10:42
Exactly ... Related: https://stackoverflow.com/questions/22519784/how-do-i-convert-an-existing-callback-api-to-promises — Jonas Wilms, May 29 '19 at 10:49

how to avoid the explicit-construction anti-pattern and still keep indentation/callbacks low

1 Answers1