Swift-function: Copy directory recursively

Question

Task description:

Implement a function, which accepts two paths: The path to a source-directory & the path to a target-directory. The function shall return true if the directory has become copied successfully. The function shall return false if the copy failed or if something has become specified, which isn't a directory.

• It the source exists and is readable, the target is writeable and the copy has worked, then the function shall return true.
• In all other cases the function shall return false.

My implementation:

struct ContentView: View {
    var docDir: URL? = nil
    var srcDir = ""
    var targetDir = ""
    
    init() {
        self.docDir = FileManager.default.urls(for: .documentDirectory,
                                               in: .userDomainMask).first
        self.srcDir = self.docDir?.appendingPathComponent("data_01").path ?? ""
        self.targetDir = self.docDir?.appendingPathComponent("data_02").path ?? ""
    }
    
    var body: some View {
        VStack(spacing: 20) {
            Button("Copy Directory") {
                print(copyDirectory(source: srcDir ?? "", target: targetDir ?? ""))
            }
            
            Button("Read Content of Directory") {
                do {
                    let items = try FileManager.default.contentsOfDirectory(atPath: targetDir)
                    for (i, item) in items.enumerated() {
                        print("\(i): \(item)")
                    }
                } catch {
                    print(error.localizedDescription)
                }
            }
        }
        .padding()
    }
    // ************* THE ACTUAL FUNCTION ********************************************
    func copyDirectory(source: String, target: String) -> Bool {
        var isDirectory: ObjCBool = true
        guard FileManager.default.fileExists(atPath: source, isDirectory: &isDirectory) == true else {
            print("Error: File does not exist")
            return false
        }
        
        guard FileManager.default.isReadableFile(atPath: source) == true else {
            print("Error: Source is not readable")
            return false
        }
        
        guard FileManager.default.isWritableFile(atPath: target) == true else {
            print("Error: Target is not writeable")
            return false
        }
        
        do {
            try FileManager.default.copyItem(atPath: source,
                toPath: "\(target)/dir_\(Int(Date.now.timeIntervalSince1970))")
        } catch {
            print("Error: Writing file failed")
            print(error.localizedDescription)
            return false
        }
        
        return true
    } // ****************************************************************************
}

Most important is the part within the comments.

What's your opinion about my approach? Especially the validation part? Are the guards a good choice?

What would you have done differently and why?

Answer 1

A few thoughts:

1. You are supplying the isDirectory parameter of fileExists(atPath:isDirectory:), but do not check its result. If the intent is to check to see if it is a directory, you should check the resulting isDirectory value. Note, it does not matter what this value was before you call fileExists, but only what it was after you call it:

   var isDirectory: ObjCBool = false
   
   guard
       FileManager.default.fileExists(atPath: source, isDirectory: &isDirectory), 
       isDirectory.boolValue 
   else {
       print("Error: Source does not exist or is not a directory")
       return false
   }
   

2. The test of isReadableFile is redundant (and introduces a race). It is better to just attempt the copy, and if it succeeds, it is readable. If it isn’t readable, the copy will fail. The isReadableFile test offers no additional utility.

   Now, if part of the requirement is to confirm that it was a directory, then fine, you have to do that. (It begs the question why you would not simply allow this function to copy individual files, too, but that is a separate question.) However, one should generally avoid tests already performed as part of the API call.

3. Your implementation is not copying to the target, but rather some timestamped subdirectory. Furthermore, it appears to require the target directory already exists.

   Neither of these requirements is part of the stated “task description”. In the absence of any explicit instructions to that effect, I would consider it an error if a developer just randomly introduced this in the implementation.

   Given the way the problem statement was worded, if source was “/…/foo” and the target was “/…/bar”, I would presume the correct behavior would be to:

   • confirm that foo is a directory and that it exists; and
   • that files/folders within foo will be copied from foo to bar, e.g., /…/foo/a.txt will be copied to /…/bar/a.txt, /…/foo/1/b.txt will be copied to /…/bar/1/b.txt, etc.

   That having been said, the “task description” says nothing with respect to what should happen if bar folder already exists. (And I do not think that the timestamped folder name is a good assumption to get around this.) Worse it says nothing if the bar directory already has some files. What if there are files bearing different file names? Do we merge these existing files with the copied ones? Worse, what if there are files bearing the same name? Should they be overwritten? Should it fail? Should it give the copied files unique/different names?

   The bottom line is that you should clarify the task before you start programming. Or, if you do not have that opportunity, make some reasonable assumptions and document that in your implementation and unit tests. But if you go rogue and introduce some of your own (non-standard) solutions, this may not be looked upon kindly.

   FWIW, the default behavior of copyItem is to perform the recursive copy, but fail if either (a) the destination folder already exists; or (b) the destination’s parent folder does not already exist. Assuming this behavior is acceptable, then the implementation might look like this:

   func copyDirectory(source: String, target: String) -> Bool {
       // make sure the `source` exists and that it is a directory
   
       var isDirectory: ObjCBool = false
   
       guard
           FileManager.default.fileExists(atPath: source, isDirectory: &isDirectory),
           isDirectory.boolValue
       else {
           print("Error: Source does not exist or is not a directory")
           return false
       }
   
       // create the parent directory of the `target` if it does not already exist
   
       let url = URL(filePath: target).deletingLastPathComponent()
       try? FileManager.default.createDirectory(at: url, withIntermediateDirectories: true)
   
       // copy, but will only succeed if `target` directory does not already exist
   
       do {
           try FileManager.default.copyItem(atPath: source, toPath: target)
       } catch {
           print("Error: Writing file failed")
           print(error)
           return false
       }
   
       return true
   }
   

   Note, there is no test of isWritableFile, either. The default behavior of copyItem is to copy the source to the target if the target does not exist. We are relying on the default copyItem behavior.

4. Given a “task description” to write a routine to perform a recursive file-copy routine, there is a chance that they are not just looking to see if you know that copyItem performs recursive copying, but to see if you can write your own implementation. Lots of interview questions and programming tests are less interested in your API knowledge than your awareness of algorithm designs and implementations.

5. Especially if you find yourself writing your own recursive copy routine, you may want to clarify a few edge-cases:

   • Is it safe to assume that the target will never be within the source? If not, you might want to make sure that you do not lazily navigate through the directory tree or else you might start some undesired recursive re-copying of files.
   • Especially if this is for a macOS target, should you follow symbolic links? Or simply create a new link to point wherever the current link points?
   • Are we asking for access to directories outside of the app sandbox, and if so, do we need to generate the UI to request permission or should we just fail for now?

   Often, acknowledging these sorts of risks and/or outlining the simplifying assumptions is sufficient to illustrate your mastery. But be aware of these concerns if you have to write your own implementation.

6. A very minor observation, but note that instead of printing of error.localizedDescription to the console, I print the full error. The localizedDescription is great for the UI, but lacks the diagnostic information that we expect/need in the console.

7. As Apple now advises we use file URLs:

   URLs are the preferred way to refer to local files.

   So, you might want to confirm that they really want you to use path strings, and not file URLs.

8. You asked:

   Are the guards a good choice?

   Where you need validation tests, guard is generally preferable to if (because it enforces the early exit).

   But where possible, eliminate tests that introduce unnecessary race conditions. We generally prefer to attempt an action and see if an error is thrown than having two steps, one to see if an action can be performed and another to perform the action.

9. As an aside, I know they asked you to return a Boolean result, but I have to say that I think that is a questionable design. I think it’s better to throw an error, so that the caller can decide whether it only cares about success vs. failure, or perhaps it might want to show a meaningful error in the UI. Do whatever the task requires, but perhaps acknowledge the benefits of meaningful error reporting rather than a mere success/failure.