4
\$\begingroup\$

I have a simple WinForm application that copies all files and directories from three folders into a fourth one. All folders are chosen/entered by the user.
Security is not a concern.

In order to prevent the application from visibly hanging I added a BackgroundWorker that updates the values of a ProgressBar. However since the task of creating directory structure and copying files isn't all that simple I fear I might have written a poor cancel handling.

Secondary concern - I might have gone overboard with the exception handling.

Apart from that ANY feedback, review or suggestion for improvement is greatly appreciated!

Some background:
The ProgressBar is named "pbMain"
The three TextBoxes that are the origin of the copy operation are named "tbECheckPath", "tbCorePath" and "tbServicePath"
There is a List<TextBox> DebugTextBoxes variable that holds all the three mentioned TextBoxes.

GitHub of the project.
The code in Pastebin

The code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.IO;
using System.Threading;
using System.Windows.Forms;

namespace PublishForQA
{
    public partial class FormProgressBar : Form
    {
        static FormPublisher formPublisher = (FormPublisher)Form.ActiveForm;
        static FormProgressBar formProgressBar;
        static List<TextBox> debugTextBoxes = formPublisher.DebugTextBoxesList;
        static BackgroundWorker backgroundWorker = new BackgroundWorker();
        static DoWorkEventArgs WorkArgs;
        static int TotalOperationsCount;
        static int CurrentOpeartionCount;

        public FormProgressBar()
        {
            TotalOperationsCount = 0;
            CurrentOpeartionCount = 0;
            InitializeComponent();
            formProgressBar = this;
            backgroundWorker.DoWork += backgroundWorker_DoWork;
            backgroundWorker.ProgressChanged += backgroundWorker_ProgressChanged;
            backgroundWorker.RunWorkerCompleted += backgroundWorker_RunWorkerCompleted;
            backgroundWorker.WorkerReportsProgress = true;
            backgroundWorker.WorkerSupportsCancellation = true;
            backgroundWorker.RunWorkerAsync();
        }

        private void backgroundWorker_DoWork(object sender, DoWorkEventArgs e)
        {
            WorkArgs = e;
            pbMain.Maximum = GetOperationsCount();
            CopyFilesAndDirectories();
        }

        private void backgroundWorker_ProgressChanged(object sender, ProgressChangedEventArgs e)
        {
            pbMain.Value = e.ProgressPercentage;
        }

        private void backgroundWorker_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
        {
            if (e.Error != null)
            {
                MessageBox.Show("An error occurred during copying:" + Environment.NewLine + e.Error.Message, "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                Cleanup();
            }
            else if (e.Cancelled)
            {
                MessageBox.Show("Copy operation aborted.", "Abort", MessageBoxButtons.OK, MessageBoxIcon.Exclamation);
                Cleanup();
            }
            else
            {
                MessageBox.Show("Copy operation completed successfully!", "Operation success", MessageBoxButtons.OK, MessageBoxIcon.Information);
                Cleanup();
            }
        }

        private void btnCancel_Click(object sender, EventArgs e)
        {
            backgroundWorker.CancelAsync();
        }

        /// <summary>
        /// Checks if "CancellationPending" is true and if it is aborts the thread and sets
        /// the DoWorkEventArgs's Cancel property to true.
        /// </summary>
        static void CheckForCancel()
        {
            if (backgroundWorker.CancellationPending)
            {
                WorkArgs.Cancel = true;
                Thread.CurrentThread.Abort();
            }
        }

        /// <summary>
        /// Marks the BackgroundWorker to be disposed and closes the progress bar form.
        /// </summary>
        static void Cleanup()
        {
            backgroundWorker.Dispose();
            formProgressBar.Close();
        }

        /// <summary>
        /// Counts all the directories that need to be created and all the
        /// files that need to be copied from each designated folder.
        /// </summary>
        /// <returns></returns>
        public static int GetOperationsCount()
        {
            formProgressBar.lblCurrentOperation.Text = "Counting the total number of operations...";
            foreach (var tb in debugTextBoxes)
            {
                CheckForCancel();
                TotalOperationsCount += Directory.GetFiles(tb.Text, "*", SearchOption.AllDirectories).Length;
                CheckForCancel();
                TotalOperationsCount += Directory.GetDirectories(tb.Text, "*", SearchOption.AllDirectories).Length;
                formProgressBar.lblCurrentPath.Text = tb.Text;
            }

            return TotalOperationsCount;
        }

        /// <summary>
        /// Recreates the directory structure at the target location and copies all files from the source recursively.
        /// </summary>
        public static void CopyFilesAndDirectories()
        {
            foreach (var tb in debugTextBoxes)
            {
                CheckForCancel();
                //If there is a task name provided we add a backslash, otherwise the QA Folder path's
                //last backslash will suffice.
                string destinationPath = formPublisher.tbQAFolderPath.Text + ((formPublisher.tbTaskName.Text.Length > 0) ? formPublisher.tbTaskName.Text + "\\" : string.Empty);

                //We set the name of the destination folder, depending
                //on the TextBox we are iterating over.
                switch (tb.Name)
                {
                    case "tbECheckPath":
                        destinationPath += "E-Check\\";
                        break;
                    case "tbCorePath":
                        destinationPath += "E-CheckCore\\";
                        break;
                    case "tbServicePath":
                        destinationPath += "E-CheckService\\";
                        break;
                    default:
                        break;
                }

                if (!CreateDirectoryStructure(tb.Text, destinationPath)) return;
                if (!CopyFiles(tb.Text, destinationPath)) return;
            }
        }

        /// <summary>
        /// Gets all the directories in a target path and recreates the same
        /// directory structure in the destination path.
        /// </summary>
        /// <param name="sourcePath">The path from which to read the directory structure.</param>
        /// <param name="destinationPath">The path where to recreate the directory structure.</param>
        /// <returns>"True" if the operation was successful, "false" if an exception was raised.</returns>
        public static bool CreateDirectoryStructure(string sourcePath, string destinationPath)
        {
            formProgressBar.lblCurrentOperation.Text = "Creating directory structure...";
            //These variables will hold the current source and target path of the "for" iteration.
            //They will be used to show more information in the exception catching.
            string sourceDir = FormPublisher.ErrorBeforeDirectoryLoop;
            string targetDir = FormPublisher.ErrorBeforeDirectoryLoop;
            try
            {
                //First we create the directory structure.
                Directory.CreateDirectory(destinationPath);
                foreach (string dirPath in Directory.GetDirectories(sourcePath, "*", SearchOption.AllDirectories))
                {
                    CheckForCancel();
                    sourceDir = dirPath;
                    targetDir = dirPath.Replace(sourcePath, destinationPath);
                    formProgressBar.lblCurrentPath.Text = targetDir;
                    Directory.CreateDirectory(targetDir);
                    CurrentOpeartionCount++;
                    backgroundWorker.ReportProgress(CurrentOpeartionCount);
                }
                return true;
            }
            #region catch block
            catch (UnauthorizedAccessException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("The caller does not have the required permission for \"" + targetDir + "\".", sourceDir, targetDir, ex), "Unauthorized Access Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (ArgumentNullException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("The path passed for directory creation is null.", sourceDir, targetDir, ex), "Argument Null Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (ArgumentException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("The path passed for directory creation is invalid.", sourceDir, targetDir, ex), "Argument Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (PathTooLongException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("Cannot create target directory, path is too long.", sourceDir, targetDir, ex), "Path Too Long Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (DirectoryNotFoundException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("The path passed for directory creation could not be found.", sourceDir, targetDir, ex), "Directory Not Found Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (IOException ex)
            {
                //IO Exception can be either the passed path is a file or the network name is not known.
                //Since we have previous checks in place to make sure the path is a directory,
                //the second possible error is shown.
                MessageBox.Show(ExceptionMessageBuilder.Directory("The network name is not known.", sourceDir, targetDir, ex), "IO Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (NotSupportedException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("The path passed contains an illegal colon character.", sourceDir, targetDir, ex), "Not Supported Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (ThreadAbortException)
            {
                Thread.ResetAbort();
                return false;
            }
            catch (Exception ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.Directory("Unexpected exception occurred:" + Environment.NewLine + ex.Message, sourceDir, targetDir, ex), "Unexpected Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            #endregion
        }

        /// <summary>
        /// Copies all files from the target path to the destination path,
        /// overriding any existing ones.
        /// </summary>
        /// <param name="sourcePath">The path from which to copy all the files.</param>
        /// <param name="destinationPath">The path where to copy all the files.</param>
        /// <returns>"True" if the operation was successful, "false" if an exception was raised.</returns>
        public static bool CopyFiles(string sourcePath, string destinationPath)
        {
            formProgressBar.lblCurrentOperation.Text = "Copying files...";
            //These variables will hold the current source and target path of the "for" iteration.
            //They will be used to show more information in the exception catching.
            //But first they are set to the string used to indicate an error before the loop.
            string sourceFile = FormPublisher.ErrorBeforeFileLoop;
            string targetFileDir = FormPublisher.ErrorBeforeFileLoop;
            try
            {
                //We copy all files, overwriting any existing ones.
                foreach (string filePath in Directory.GetFiles(sourcePath, "*", SearchOption.AllDirectories))
                {
                    CheckForCancel();
                    sourceFile = filePath;
                    targetFileDir = filePath.Replace(sourcePath, destinationPath);
                    formProgressBar.lblCurrentPath.Text = filePath;
                    File.Copy(filePath, targetFileDir, true);
                    CurrentOpeartionCount++;
                    backgroundWorker.ReportProgress(CurrentOpeartionCount);
                }
                return true;
            }
            #region catch block
            catch (UnauthorizedAccessException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("The caller does not have the required permission for \"" + targetFileDir + "\".", sourceFile, targetFileDir, ex), "Unauthorized Access Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (ArgumentNullException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("Either the source or destination file paths are null.", sourceFile, targetFileDir, ex), "Argument Null Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (ArgumentException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("Either the source or destination file paths are invalid.", sourceFile, targetFileDir, ex), "Argument Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (PathTooLongException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("Either the source or destination file paths are too long.", sourceFile, targetFileDir, ex), "Path Too Long Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (DirectoryNotFoundException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("Either the source or destination file paths could not be found.", sourceFile, targetFileDir, ex), "Directory Not Found Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (NotSupportedException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("Either the source or destination file paths are invalid.", sourceFile, targetFileDir, ex), "Not Supported Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (FileNotFoundException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("\"" + sourceFile + "\" was not found.", sourceFile, targetFileDir, ex), "File Not Found Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (IOException ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("An I/O error has occurred.", sourceFile, targetFileDir, ex), "IO Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            catch (ThreadAbortException)
            {
                Thread.ResetAbort();
                return false;
            }
            catch (Exception ex)
            {
                MessageBox.Show(ExceptionMessageBuilder.File("An unexpected exception has occurred:" + Environment.NewLine + ex.Message, sourceFile, targetFileDir, ex), "Unexpected Exception", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            #endregion
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I would rather see a pattern where you pass the DoWorkEventArgs e and just cancel in the worker. But if what you have works then OK. \$\endgroup\$ – paparazzo Feb 25 '18 at 18:00
  • \$\begingroup\$ @Paparazzi, currently it works just fine but I am looking to improve it. However I am afraid I do not understand how I can pass the DoWorkEventArgs and cancel in the worker, given the operations that need to be performed. \$\endgroup\$ – Ivan Stoychev Feb 25 '18 at 20:11
  • \$\begingroup\$ The Microsoft documentation is littered with passing that argument. \$\endgroup\$ – paparazzo Feb 25 '18 at 20:18
  • \$\begingroup\$ @Paparazzi, actually it was the "canceling in the worker" part that got me confused but I'll look more into it, thanks! \$\endgroup\$ – Ivan Stoychev Feb 27 '18 at 19:16
2
\$\begingroup\$

1) The behavior and timing of Thread.Abort method is hard to predict, so you should avoid using it unless you 100% know what you are doing. In my experience, it is only used as last resort measure in rare exotic circumstances. It should not be used as part of regular workflow of your app. In you case, for example, I highly doubt that aborting a thread does anything useful, since File.Copy is a native call, and it probably can't be aborted by CLR.

2) Last time I checked there was no File.Copy method that supports cancellation. But that was a long time ago, maybe things have changed since then. But if it is still the case, there are other options:

3) In general you should try to separate business logic from UI. There should be a different class that does the copying, and your Form should be unaware of its implementation details.

4) Why are you using static fields? Don't, unless you have to for some reason.

5) IMHO, nowadays background worker is rarely used (async programming made it kind of obsolete), but so is WinForms. So apart from your code being somewhat out of date, your use of background worker looks alright to me, I would not call it overly complex.

6) As for exceptions:

  • Validate input beforehand where possible, instead of waiting for exceptions to occur. For example, you can easily check whether the file path is null or whether it has correct format before user tries to copy anything. This can drastically reduce the amount of exceptions your have to catch later on.
  • Think about who your users are. Do they really need to know what went wrong exactly? Can they do something about it? If so, then yes, the way you handle exceptions is correct. If no, then instead of generating 100500 different message, you might just want to show a single one that would allow user to easily send an underlying exception and stack trace to your emial :)
\$\endgroup\$
  • \$\begingroup\$ Good points, thanks for the feedback! About Thread.Abort I only use it because I do not know how I can signal to the second thread or the BackGroundWorker to stop working and fire the backgroundWorker_RunWorkerCompleted event mid-execution. You are right about 3, it totally slipped my mind. About 2 - I do not need to cancel the current file copy operation (but it's nice to know it's possible, I'll look into those links) I only want to tell the program to not copy any more files after "this one" and instead stop. \$\endgroup\$ – Ivan Stoychev Feb 27 '18 at 19:20
  • \$\begingroup\$ @IvanStoychev then all you need to do is return from your CopyFilesAndDirectories whenever cancellation is requested. Just replace CheckForCancel(); with if (CheckForCancel()) return; \$\endgroup\$ – Nikita B Feb 28 '18 at 12:45
  • \$\begingroup\$ Hmmmm, I guess it's safer, but it just seems uglier to me. Aside from that, is the rest really OK? I thought I was over-complicating the BackGroundWorker and maybe going a bit overboard with the catch block. \$\endgroup\$ – Ivan Stoychev Feb 28 '18 at 21:55
  • \$\begingroup\$ @IvanStoychev I've updated my answer. \$\endgroup\$ – Nikita B Mar 1 '18 at 9:03
  • \$\begingroup\$ Thank you very much! I use static fields to make it easier to use said resources. Otherwise I'd have to deal with instantialisation but I ever only must have 1 copy of said type, so... \$\endgroup\$ – Ivan Stoychev Mar 4 '18 at 10:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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