Cleaner Way to Structure Asynchronous Code

Question

The purpose of the code is to allow the user to select directories that will be crawled recursively in order to find particular files or file types to analyze en masse. The thing to bear in mind is that if C:\ is already selected, then any child path of C:\ that the user tries to select will be discarded. The entire process of the script can be broken down into the following steps:

1. The user selects a directory or directories via a dialog box.
2. The script then compares the selection to whatever directories might already be stored in an array in the configuration file, which is a JSON object.
3. If the selection is invalid, the selection is discarded. / If the selection is valid, the configuration file is modified accordingly.

Obviously promises are ideal, so I'm using Node's fs.promises. However, the tricky bit is that, although they're asynchronous, parts of the code must behave synchronously. For example, it would be a waste of resources to retrieve the array from the configuration file if the dialog box is cancelled by the user.

I'm completely new to promises and everything else that entails, so I'm wondering whether or not there's an even better way to structure the asynchronous code or the code in general? The code works fine the way it is, but can its structure/readability and or efficiency be improved?

async function userSelectsArchives()
{
  try {
    const r1 = await dialog.showOpenDialog(remote.getCurrentWindow(), {
      title: 'Select Archives to Analyze',
      buttonLabel: 'Select Archives',
      defaultPath: 'c:\\',
      properties: [ 'openDirectory', 'multiSelections' ]
    })

    if (r1.canceled) throw new Error('Dialog cancelled.')

    const
      r2 = await fsp.readFile('./config.json', 'utf8'),
      config = JSON.parse(r2),
      archivesOG = config.archives

    for (const dir of r1.filePaths){
      // Disregard user input if parent path or exact match already exists.
      if (
        config.archives.reduce((a, v) => {
          return a || dir.indexOf(v.slice(-1) === '\\' ? v : (v + '\\')) === 0
        }, false)
        || config.archives.includes(dir)
      ) continue
      // Remove child paths of user input in config.
      config.archives = config.archives.filter(
        v => v.indexOf(dir.slice(-1) === '\\' ? dir : (dir + '\\')) !== 0
      )
      // Add user input to archive.
      config.archives.push(dir)
    }

    // Verify that changes have occurred or throw error.
    if (archivesOG === config.archives) throw new Error('Invalid selection.')

    await fsp.writeFile('./config.json', JSON.stringify(config, null, 2), 'utf8')

    displayDirs(config.archives)
  }
  catch (error) {
    console.error(error)
  }
}

Answer 1

Tips for JS code optimization:

Variable names

Give your variables meaningful names:

• const r1 = await dialog.showOpenDialog(...) can be renamed to dirs or dirList (representing selected directories)
• const r2 = await fsp.readFile('./config.json', 'utf8'), config = JSON.parse(r2), where r2 is supposed to be a config file path. Let's rename it to say
  const config_file = await fsp.readFile('./config.json', 'utf8'), config = JSON.parse(config_file)

Extract function:

Repeated expression <string>.slice(-1) === '\\' ? <string> : (<string> + '\\') can be extracted into a reusable routine (named or anonymous function):

var asDirPath = function(path) {
    return path.slice(-1) === '\\' ? path : (path + '\\');
};

Substitute algorithm:

The whole condition (combination of array.reduce and array.includes) can be easily substituted with a more efficient Array.some() approach (to check for any match between current directory path dir and config.archives file/dir paths):

for (const dir of dirList.filePaths){
    // Disregard user input if parent path or exact match already exists.
    if (config.archives.some(path => {
            return dir.indexOf(asDirPath(path)) === 0 || path === dir
    })) continue

Answer 2

Some suggestions:

1. Since this is an async function, it seems likely the caller may want to know when it's done and if it succeeded. To do that, you need to reject upon error. That means rethrow after logging inside your catch. This allows the error to get back to the caller as a rejected promise.

2. Simplify by changing config.archives.reduce() to config.archives.some(). This will also be faster in cases where there is a match because .some() will short-circuit the iteration as soon as a match is found.

3. Change return dir.indexOf(v.slice(-1) === '\\' ? v : (v + '\\')) === 0; to return dir.indexOf(v) === 0;. It appears the point of this code is to allow partial matches where v is a subset of dir, but starts matching at the beginning. If that's the case and if I understood what you're trying to do here, then you don't need to make sure that v has a trailing slash on it as that won't change the outcome.

4. Remove || config.archives.includes(dir). It appears that is redundant since a full match will have already been found in the previous iteration through config.archives.

5. Coding style. I prefer if (condition) { do something } rather than if (condition) continue as I just think it's easier to read and follow.

6. You have a lot of references to config.archives. I would suggest either just using archivesOG instead or make a shorter named local variable such as just archives that you can refer to everywhere except when assigning back to it.

7. r1 and r2 could have much more meaningful names.

8. The code dir.indexOf(v.slice(-1) === '\\' ? v : (v + '\\')) === 0; begs to be put in a function with a meaningful name. It takes a bit of a pause when reading this code to figure out what that's doing. If it has a meaningful function name such as normalizePathEnd() or something like that, the code will be a lot easier to read without having to follow the detail of the string manipulation. Also, your code as you show it has two copies of this concept which also begs to be in a utility function.