HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpMinor

Copying file dependencies

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filecopyingdependencies

Problem

I have written the following code, and it works great, but I'm not satisfied. I really love the book "Clean Code" by Uncle Bob and I try to apply all his principles when coding, but I'm having a tough time cleaning this abomination. I call it an abomination because I'm sure everyone new to this method will spend more than 10 seconds trying to figure out what's going on. It's not easy to read. So I'm hoping for input on how to make it easier to read.

private void CopyFileDependencies(FileInfo fileInfo, string destinationPath, int recoveryAttempts = 1)
{
  try
  {
    foreach (var file in Directory.GetFiles(fileInfo.DirectoryName, "*.*", SearchOption.AllDirectories))
    {
      File.Copy(file, file.Replace(fileInfo.DirectoryName, destinationPath), true);
    }
  }
  catch (IOException)
  {
    if (recoveryAttempts > 0)
    {
      KillProcess(fileInfo);
      CopyFileDependencies(fileInfo, destinationPath, --recoveryAttempts);
    }
    else
    {
      throw;
    }
  }
}

Solution

Why is a method that is ostensibly about "copying file dependencies" killing a process?

It also seems awkward to use recursion for something like this, especially from the catch block. Maybe it's just me?

I would get rid of the recursion and the process killing, and let the calling code higher up deal with that. This method should just attempt to copy the files from a to b, and the calling code can run it as many times as it wants, handle failures by killing a process or whatever, etc.

Context

StackExchange Code Review Q#123528, answer score: 3

Revisions (0)

No revisions yet.