3
\$\begingroup\$

I have written a simple copy file code, called 'Amature SVN', kindly review my code.

using System;
using System.Collections.Generic;
using System.Data;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.IO;

namespace AmatureSVN
{
    public partial class Form1 : Form
    {

    public Form1()
    {
        InitializeComponent();

    }

    private void Form1_Load(object sender, EventArgs e)
    {

    }

    private void btnSelectFolder_Click(object sender, EventArgs e)
    {
        DialogResult result = dirBrowserDlg.ShowDialog();
        if (result == DialogResult.OK)
        {
            txtSelectedFolder.Text = dirBrowserDlg.SelectedPath;
        }
    }

    private void btnSelectFiles_Click(object sender, EventArgs e)
    {
        DialogResult result = openFileDialog1.ShowDialog();
        if (result == DialogResult.OK)
        {
            string[] selectedFiles = openFileDialog1.FileNames;

            foreach (string str in selectedFiles)
            {
                //unique items
                if (listboxSelectedFiles.Items.Contains(str)) continue;

                listboxSelectedFiles.Items.Add(str);
            }
        }
    }

    private void btnCopy_Click(object sender, EventArgs e)
    {
        string backupDir = CreateBackupDir();

        //if dir not created return
        if (string.IsNullOrEmpty(backupDir)) return;

        CopyFilesToDir(backupDir);

        //Create Readme
        if (!string.IsNullOrEmpty(txtReadMe.Text))
            CreateReadMe(backupDir);
    }

    private void CreateReadMe(string backupDir)
    {
        using (StreamWriter writer = new StreamWriter(backupDir + @"\ReadMe.txt",false))
        {
            try
            {
                writer.Write(txtReadMe.Text);
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }
    }

    string CreateBackupDir()
    {            
        //create backup folder at selected location
        if (!string.IsNullOrEmpty(txtSelectedFolder.Text))
        {
            /*get todays date and time
            date will we folder for today
            time will be subfolder*/
            string nowTime = DateTime.Now.ToString("dd MMM yyyy") + @"\" + DateTime.Now.ToString("hh mm");

            string backupDir = txtSelectedFolder.Text + @"\" + nowTime;

            //create dir
            try
            {
                if (!Directory.Exists(backupDir))
                {
                    Directory.CreateDirectory(backupDir);
                }
                return backupDir;
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
                return "";
            }
        }

        return "";
    }

    void CopyFilesToDir(string backupDir)
    {
        foreach (string file in listboxSelectedFiles.Items)
        {
            //copy file with overwrite option
            try
            {
                File.Copy(file, backupDir + @"\" + file.Split('\\').Last(), checkBoxOverwrite.Checked);
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

You mix UI and logic together. You should extract all the copy code into a separate class and pass in the parameters from the form. This will make your code more reusable (right now you can't write an automated copy program which takes file names from a config file for example).

When you refactor then let your exceptions bubble up and let the caller (the UI in this case) deal with them.

CreateBackupDir should probably be more something along the lines EnsureBackupDirExists (create if not exists).

Something along these lines:

public class BackupUtility
{
    private string _BackupDirectoryBase;
    private bool _OverWriteExistingFiles;

    public BackupUtility(string backupDirectoryBase, bool overwriteExistingFiles)
    {
        if (string.IsNullOrEmpty(backupDirectoryBase))
            throw new ArgumentException("Backup directory must not be null or empty");
        _BackupDirectoryBase = backupDirectoryBase;
        _OverWriteExistingFiles = overwriteExistingFiles;
    }

    public void BackupFiles(IEnumerable files, string readmeContent)
    {
         var actualBackupDir = EnsureBackupDirExists();
         CopyFilesToBackupDir(actualBackupDir, files);
         CreateReadmeIfRequired(actualBackupDir, readmeContent);
    }

    private string EnsureBackupDirExists()
    {
        /* get todays date and time
             - date will we folder for today
             - time will be subfolder */
        string today = DateTime.Now.ToString("dd MMM yyyy")
        string time = DateTime.Now.ToString("hh mm");

        string backupDir = Path.Combine(_BackupDirectoryBase, today, time);

        if (!Directory.Exists(backupDir))
        {
            Directory.CreateDirectory(backupDir);
        }
        return backupDir;
    }

    private void CopyFilesToBackupDir(string actualBackupDir, IEnumerable files)
    {
        foreach (string file in file)
        {
           string fileName = Path.GetFileName(file);
           File.Copy(file, Path.Combine(actualBackupDir, fileName), _OverwriteExistingFiles);
        }
    }

    private void CreateReadMeIfRequired(string actualBackupDir, string readmeContent)
    {
        if (string.IsNullOrEmpty(readmeContent))
            return;

        var readmeFile = Path.Combine(actualBackupDir, "ReadMe.txt");
        File.WriteAllText(readmeFile, readmeContent); // will overwrite if exist
    }
}
\$\endgroup\$
5
\$\begingroup\$

There isn't much too review. A few thing things that can be improved are:

  1. Use Path.Combine() instead of building file paths manually.
  2. Use String.Empty instead of "".
  3. Reduce nesting by inverting if statements, e.g. by replacing

    if (result == DialogResult.OK)
    {...}
    

    with

    if (result != DialogResult.OK)
        return;
    
\$\endgroup\$
  • \$\begingroup\$ @ss7don, not really, its pretty clear. It says if <condition> then nothing happens. While nested ifs kind of force you to read through the whole execution path to figure out if something happens. It doesn't matter when you have 3-line-methods, but it helps, when methods are somewhat larger. \$\endgroup\$ – Nikita B Oct 7 '13 at 12:53

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.