Amature SVN review

Question

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);
 }
 }
 }
}

Answer 1

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
 }
}

Answer 2

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;
   

• Nikita B: @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.