2
\$\begingroup\$

My goal is to create a function that safely renames files by adding numeric increments if the file is found at the destination, without using a third party library.

Having a source file foo.bar and an existing destination foo.bar the function would rename using foo (1).bar, foo (2).bar, foo (3).bar, etc.

I have to fs.lstat() first because relying on fs.rename() failing only works on win32 since linux, darwin will follow the posix conventions and blindly overwrite destination files. I realize there is a non-zero chance that the file could be moved before the rename, unfortunately, I haven't found an asynchronous friendly way to do it.

So this is what I came up with, and it may be rather unconventional because I'm relying on a catch() block to do the work.

While this works, I'm leaning on your experience to point out any pitfalls and looking for a better, more syntactically clear way to handle this, advice appreciated!

const fs   = require('fs')
const path = require('path')

const safe = async (src,dst) => {
    
    let x       = 1
    let target  = dst
    let renamed = false

    while(x < 32) {

        await fs.promises.lstat(target)
        .then(() => target = path.join( path.dirname(dst), path.basename(dst, path.extname(dst)) + ' (' + x + ')' + path.extname(dst) ))
        .catch(async e => {

            console.log(e)
        
            await fs.promises.rename(src,target)
            .then(() => renamed = true)
            .catch(e => console.log(e))
        
        })

        if(renamed === true) break

        x++
    }
    
}

safe('/source/foo.bar','/destination/foo.bar')
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • On the logical side, having two different conditions to break a loop in two different places makes the code harder to follow. Without changing the flow of the current implementation, why not while (x < 32 && !renamed) {, for example? I'd generally avoid using flags at all, in favour of break and continue (but with the current mix of callbacks that's not an option) and prefer for to while where the range of values is known up-front.

  • safe returns a promise that resolves whatever happens. Did the file get renamed? Did we run out of numerical suffixes? It's not clear, which means that any caller can't take any action based on the outcome.

  • Perhaps most importantly it's clear from the code (and the edits to this question!) is that you don't have a solid understanding of promises. The async/await syntax and .then are interoperable, it's all just promises under the hood, but sticking to one or the other makes the code much easier to follow. When nesting promises,

    onePromise
      .then(async () => {
        await anotherPromise;
      });
      .then(() => console.log("both done");
    

    works because the .then callback returns a promise (as async functions always do) which gets resolved as part of the existing promise chain. So does:

    onePromise
      .then(() => {
        return anotherPromise;
      });
      .then(() => console.log("both done");
    

    for exactly the same reason, but this won't:

    onePromise
      .then(() => {
        anotherPromise;
      });
      .then(() => console.log("both done");
    

    because the inner promise isn't part of the outer chain. You can also use the chaining, so:

    onePromise
      .then(() => 
        return anotherPromise.then((value) => console.log("value"));
      })
      .then(() => {
        console.log("both done");
      });
    

    can be rewritten:

    onePromise
      .then(() => 
        return anotherPromise;
      })
      .then((value) => {
        console.log("value")
        console.log("both done");
      });
    
    

First, we can extract a function that creates a new destination based on the initial destination and the suffix to add:

const addSuffix = (destination, suffix) => {
  const extension = path.extname(destination)
  const filename = path.basename(destination, extension)
  return path.join(
    path.dirname(destination),
    `${filname} (${suffix})${extension}`
  )
}

Second, let's tackle the rename of a given source to a destination, without worrying about what should happen if that doesn't work out. Note that the lstat error is expected (interpreted as "that destination is available") but any error on the rename is not expected and left to the caller. This gives three outcomes:

  1. promise resolves true: file was renamed
  2. promise resolves false: destination was occupied
  3. promise rejects: something unexpected went wrong
const rename = async (source, destination) => {
  try {
    await fs.promises.lstat(destination)
    return false
  } catch (err) {
    console.error(err)
  }
  await fs.promises.rename(source, destination)
  return true
}

Now we can rebuild a much simpler safe from these components:

const safe = async (source, destination) => {
  let target = destination

  for (let index = 1; index < 32; index++) {
    if (await rename(source, target)) {
      return target
    }
    target = addSuffix(destination, index)
  }

  throw new Error('exhausted all suffixes')
}

This has two outcomes:

  1. promise resolves: file was (eventually) renamed
  2. promise rejects: it wasn't

This lets you provide appropriate feedback to the user:

safe('/source/foo.bar','/destination/foo.bar')
  .then((destination) => console.log(`file moved to ${destination}`))
  .catch((err) => {
    console.error(err)
    process.exit(1)
  })
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.