1
\$\begingroup\$

I made a little copy tool, but my progress bar seems to be too slow. I would also be very interested in any tips and improvements or code smells.

MainForm (code behind):

public partial class MainForm : Form
{
    private FileCopy _fileCopy;
    private delegate void UpdateProgressEventHandler(object o, CopyProgressChangedEventArgs e);
    private delegate void UpdateProgressFinishedEventHandler();

    public MainForm()
    {
        InitializeComponent();
    }

    private void Button_Source_Click(object sender, EventArgs e)
    {
        FolderBrowserDialog fbd = new FolderBrowserDialog();
        if (fbd.ShowDialog() == DialogResult.OK)
        {
            textBox_SourcePath.Text = fbd.SelectedPath;
        }
    }

    private void Button_Destination_Click(object sender, EventArgs e)
    {
        FolderBrowserDialog fbd = new FolderBrowserDialog();
        if (fbd.ShowDialog() == DialogResult.OK)
        {
            textBox_DestinationPath.Text = fbd.SelectedPath;
        }
    }

    private void Button_Copy_Click(object sender, EventArgs e)
    {
        progressBar_Copy.Value = 0;

        if ((textBox_SourcePath.Text != String.Empty) &&
            textBox_DestinationPath.Text != String.Empty)
        {
            _fileCopy = new FileCopy(textBox_SourcePath.Text, textBox_DestinationPath.Text);

            _fileCopy.CopyProgressChangedEvent += FileCopy_CopyProgressChangedEvent;
            _fileCopy.CopyProgressFinishedEvent += FileCopy_CopyProgressFinishedEvent;
            _fileCopy.StartCopyAsync();
            button_Copy.Enabled = false;
            button_Copy.Text = "Cancel";
        }
        else
            MessageBox.Show("Check you Path Inputs!", "INPUT ERROR!", MessageBoxButtons.OK, MessageBoxIcon.Error);
    }

    public void FileCopy_CopyProgressChangedEvent(object sender, CopyProgressChangedEventArgs e)
    {
        if (InvokeRequired)
            Invoke(new UpdateProgressEventHandler(FileCopy_CopyProgressChangedEvent), new object[] { sender, e });

        else
        {
            var state = e.State;

            progressBar_Copy.Value = (int)state.Progress;
            label_CopyFile.Text = "Copying... " + state.Filename;
            label_FileCount.Text = "[" + state.FilesCopyied.ToString() + "/" + state.FileCount.ToString() + "]";
        }
    }

    private void FileCopy_CopyProgressFinishedEvent()
    {
        if (InvokeRequired)
            Invoke(new UpdateProgressFinishedEventHandler(FileCopy_CopyProgressFinishedEvent));
        else
        {
            progressBar_Copy.Value = 100;
            label_CopyFile.Text = "Copying Done!";
            button_Copy.Text = "Back Me Up!";
            button_Copy.Enabled = true;
        }        
    }

}

FileCopy class:

class FileCopy
{
    DirectoryInfo _sourceDictInfo;
    DirectoryInfo _destDictInfo;
    private BackgroundWorker _copyWorker;
    private List<FileInfo> _filesToCopy;
    private bool _override = true;

    public delegate void CopyProgressChangedHandler(object sender, CopyProgressChangedEventArgs e);
    public delegate void CopyProgressFinishedHandler();
    public event CopyProgressChangedHandler CopyProgressChangedEvent;
    public event CopyProgressFinishedHandler CopyProgressFinishedEvent;

    #region Events
    protected void OnCopyProgressChangedEvent(CopyProgressChangedEventArgs e)
    {
        var handler = CopyProgressChangedEvent;
        handler?.Invoke(this, e);
    }

    protected void OnCopyProgressFinishedEvent()
    {
        var handler = CopyProgressFinishedEvent;
        handler?.Invoke();
    }
    #endregion

    public FileCopy(string sourcePath, string destPath)
    {
        _filesToCopy = new List<FileInfo>();
        _copyWorker = new BackgroundWorker();
        _sourceDictInfo = new DirectoryInfo(sourcePath);
        _destDictInfo = new DirectoryInfo(destPath);
    }

    public void StartCopyAsync()
    {
        _copyWorker.DoWork += CopyWorker_DoWork;
        _copyWorker.WorkerSupportsCancellation = true;
        _copyWorker.RunWorkerAsync();
    }

    private void CopyWorker_DoWork(object sender, DoWorkEventArgs e)
    {
        CopyDirectory(_sourceDictInfo.FullName, _destDictInfo.FullName);
    }

    private void CopyDirectory(string sourcePath, string destinationPath)
    {
        CopyState state = new CopyState();
        CollectFilesToCopy(sourcePath, state, _filesToCopy);

        foreach (FileInfo file in _filesToCopy)
        {
            char[] trimChars = sourcePath.ToCharArray();
            string newPartOfDestinationDirectory = file.FullName.TrimStart(trimChars);
            string destinationDirectory = _destDictInfo.FullName;
            string targetFile = destinationDirectory + Path.DirectorySeparatorChar + newPartOfDestinationDirectory;

            try
            {
                state.Filename = file.Name;
                state.AllreadyCopiedSize += file.Length;
                state.FilesCopyied++;
                OnCopyProgressChangedEvent(new CopyProgressChangedEventArgs(state));
                File.Copy(file.FullName, targetFile, _override);
            }
            catch
            {
                // Fancy Logging Magic....
            }
        }                       
        OnCopyProgressFinishedEvent();
    }


    private void CollectFilesToCopy(string sourcePath, CopyState state, List<FileInfo> filesToCopy)
    {

        DirectoryInfo sourceDirectory = new DirectoryInfo(sourcePath);

        FileInfo[] files = null;
        try
        {
            files = sourceDirectory.GetFiles();
        }
        catch(UnauthorizedAccessException ex)
        { 
            // DO SOME LOGGING-MAGIC IN HERE...
        }

        if (files != null)
        {
            foreach (FileInfo file in files)
            {
                state.MaxSize += file.Length;
                filesToCopy.Add(file);
                state.FileCount++;
            }
        }

        DirectoryInfo[] directories = null;
        try
        {
            directories = sourceDirectory.GetDirectories();
        }
        catch(UnauthorizedAccessException ex)
        {
            // Do more logging Magic in here...
        }
        if (directories != null)
        foreach (DirectoryInfo direcotry in directories)
        {
            CollectFilesToCopy(direcotry.FullName, state, filesToCopy);
        }
    }

}

CopyState Class:

public class CopyState
{
    public string Filename { get; set; }

    private double _alreadyCopiedSize;
    public double AllreadyCopiedSize
    {
        get
        {
            return _alreadyCopiedSize;
        }
        set
        {
            Progress = (100 * (AllreadyCopiedSize / MaxSize)) == 0 ? 1 : (100 * (AllreadyCopiedSize / MaxSize));
            _alreadyCopiedSize = value;
        }
    }
    public double MaxSize { get; set; }
    public int FileCount { get; set; }
    public int FilesCopyied { get; set; }
    public double Progress { get; private set; }
}
\$\endgroup\$
2
  • \$\begingroup\$ The bottleneck of your copy tool seems to be the File.Copy API, see: File.Copy vs. Manual FileStream.Write For Copying File. \$\endgroup\$
    – t3chb0t
    Commented Aug 14, 2016 at 9:35
  • \$\begingroup\$ Closed voted as broken code, no comment left, thus I voted to leave open because I don't see the actual brokenness \$\endgroup\$
    – Pimgd
    Commented Aug 14, 2016 at 10:23

1 Answer 1

2
\$\begingroup\$

Here are my suggestions:

  • you are using BackgroundWorker which is outdated, prefer Task instead
  • a common cause for stuck progress bars is that calls happen too quickly for UI to update, you need to think percentage progress to overcome this situation

Haven't really bothered trying to fix your code :D instead I wrote the following, hopefully it'll give you some inspiration:

  • it has 2 progress bars, one for current file, one for global
  • handles sub-directories in source
  • handles cancellation

enter image description here

using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        private CancellationTokenSource _cts;
        private string _source;
        private string _target;

        public Form1()
        {
            InitializeComponent();
        }

        private void buttonSource_Click(object sender, EventArgs e)
        {
            _source = PickFolder();
            labelSource.Text = _source;
            UpdateStartButton();
        }

        private void buttonTarget_Click(object sender, EventArgs e)
        {
            _target = PickFolder();
            labelTarget.Text = _target;
            UpdateStartButton();
        }

        private async void buttonStartCancel_Click(object sender, EventArgs e)
        {
            var button = (Button) sender;
            if (button.Text == @"Start")
            {
                button.Text = @"Cancel";
                _cts = new CancellationTokenSource();
                await Task.Run((Action) Copy, _cts.Token);
            }
            else if (button.Text == @"Cancel")
            {
                button.Text = @"Start";
                _cts.Cancel();
            }
            else if (button.Text == @"Close")
            {
                Close();
            }
        }

        private void Copy()
        {
            var source = _source;
            var target = _target;
            var files = Directory.GetFiles(source, "*.*", SearchOption.AllDirectories);
            var buffer = new byte[65536];
            var i = 0;
            foreach (var path in files)
            {
                var substring = path.Substring(source.Length + 1);
                var targetPath = Path.Combine(target, substring);
                var targetDir = Path.GetDirectoryName(targetPath);
                if (!Directory.Exists(targetDir)) Directory.CreateDirectory(targetDir);

                var percent = 0;
                using (var input = File.OpenRead(path))
                using (var output = File.Create(targetPath))
                {
                    while (output.Length < input.Length)
                    {
                        if (_cts.IsCancellationRequested)
                            goto canceled; // evil :)

                        var read = input.Read(buffer, 0, buffer.Length);
                        output.Write(buffer, 0, read);

                        var percent1 = (int) Math.Floor(1.0d/input.Length*output.Length*100.0d);
                        if (percent1 > percent)
                        {
                            Invoke((Action) delegate { progressBar1.Value = percent1; });
                            percent = percent1;
                        }
                    }
                }
                var percent3 = (int) (1.0d/files.Length*(i + 1)*100.0d);
                Invoke((Action) delegate { progressBar2.Value = percent3; });
                i++;
            }

            Invoke((Action) delegate { buttonStartCancel.Text = @"Close"; });
            return;

            canceled:
            Console.WriteLine(@"Operation canceled");

            // TODO handle things as needed
        }

        private void UpdateStartButton()
        {
            buttonStartCancel.Enabled = _source != null && _target != null && _source != _target;
        }

        private static string PickFolder()
        {
            using (var dialog = new FolderBrowserDialog())
            {
                return dialog.ShowDialog() == DialogResult.OK ? dialog.SelectedPath : null;
            }
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for comment, I will take a closer look on your code later this day. At a first look I got some nice ideas from you code I will build into mine (e.g. cancelation, tasks and have to think about the progress behavior). Thanks for that. At a Second look, I see no error handling (What if File does not exists anymore, Permission denied Exception and so on.) Second I see a "goto"... no comment on this ;-) And third, I don't think your code can handle large files because of your restricted buffer. Greetings \$\endgroup\$
    – Locke
    Commented Aug 14, 2016 at 7:37
  • \$\begingroup\$ Well, cacellation is handled by the CancellationToken.ThrowIfCancellationRequested and not by using a goto ;-) \$\endgroup\$
    – t3chb0t
    Commented Aug 14, 2016 at 8:53
  • \$\begingroup\$ @Locke that was really a quick sample I worked out and I've put a TODO at bottom; obviously there's quite more that is needed to have something robust. \$\endgroup\$
    – aybe
    Commented Aug 14, 2016 at 17:57

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.