1

I have a simple program that copies files and directories from one place to another. I have it set-up that if there are any exceptions (such as if access to the path is denied) it will create a log file with the error.

I have a button that when pressed, performs the copy action. Everything works fine the first time I press the button and the log file is either created or overwritten with the appropriate error messages.

However, if I press the button a second time, the text file is not overwritten and instead the error messages append. If I close out of my program and run it again, the file is overwritten on the first button press. Any thoughts would be greatly appreciated.

target is a string filepath which I'm getting from a FolderBrowserDialog and taking the selected path and setting it to a textbox. loglist is just a simple List<string> I'm using to store the error messages from any exceptions that occur during the copy process.

public partial class Form1 : Form
{

    static List<string> logList = new List<string>();

    public Form1()
    {
        InitializeComponent();
    }


    private static void CopyAll(DirectoryInfo source, DirectoryInfo target)
    {


        if (source.FullName.ToLower() == target.FullName.ToLower())
            return;

        if (Directory.Exists(target.FullName) == false)
        {
            Directory.CreateDirectory(target.FullName);
        }





        foreach (FileInfo fi in source.GetFiles())
        {
            try
            {
                fi.CopyTo(Path.Combine(target.ToString(), fi.Name), true);
            }
            catch (Exception ex)
            {
                logList.Add(ex.Message);


            }


        }

        foreach (DirectoryInfo diSourceSub in source.GetDirectories())
        {
            DirectoryInfo nextTargetSubDir = target.CreateSubdirectory(diSourceSub.Name);
            CopyAll(diSourceSub, nextTargetSubDir);

        }



    }


    private void directoryPickerBtn1_Click(object sender, EventArgs e)
    {
        FolderBrowserDialog folderDialog = new FolderBrowserDialog();

        DialogResult folderResult = folderDialog.ShowDialog();
        if (folderResult == DialogResult.OK)
        {

            directoryTextbox1.Text = folderDialog.SelectedPath;
        }


    }

    private void directoryPickerBtn2_Click(object sender, EventArgs e)
    {
        FolderBrowserDialog folderDialog = new FolderBrowserDialog();

        DialogResult folderResult = folderDialog.ShowDialog();
        if (folderResult == DialogResult.OK)
        {
            directoryTextbox2.Text = folderDialog.SelectedPath;
        }
    }

    private void copyBtn_Click(object sender, EventArgs e)
    {
        string source = (directoryTextbox1.Text);
        string target = (directoryTextbox2.Text);



        DirectoryInfo dirSource = new DirectoryInfo(source);
        DirectoryInfo dirTarget = new DirectoryInfo(target);

        try
        {
            CopyAll(dirSource, dirTarget);

            if (logList.Count > 0)
            {
                using (StreamWriter sw = new StreamWriter(target + @"\log.txt", false))
                {
                    foreach (string error in logList)
                    {
                        sw.WriteLine(error);

                    }


                }
            }


            DialogResult result = MessageBox.Show("Copy Succeeded", "Success");
            if (result == DialogResult.OK)
            {
                string myPath = dirTarget.ToString();
                System.Diagnostics.Process prc = new System.Diagnostics.Process();
                prc.StartInfo.FileName = myPath;
                prc.Start();
            }
        }
        catch (Exception)
        {
            MessageBox.Show("Copy Failed", "Failed");
        }


    }
}

}

Matt
  • 37
  • 1
  • 7
  • The code you show creates a new file every time. Please show _all_ relevant code, starting with the button click event handler. – CodeCaster Nov 26 '15 at 16:32
  • I'm not sure if that link helps, @Mate. – CodeCaster Nov 26 '15 at 16:32
  • Do you remove the entries in loglist? or is the file actually just overwritten every time? – James Nov 26 '15 at 16:37
  • 2
    Both methods work properly and it seems the problem is with your logList that probably you only append messages to it and it always contains all messages. – Reza Aghaei Nov 26 '15 at 16:40
  • THANK YOU ALL FOR ANSWERING THIS SO QUICKLY...SO IS THE BEST! – Matt Nov 26 '15 at 16:47
  • Matt, Please don't include the answer in the question; this will be confusing for people viewing it later – James Nov 26 '15 at 16:59
  • @JamesBarrass good point. Kinda new to SO as well..will edit it again..thanks for the heads up – Matt Nov 26 '15 at 17:10

2 Answers2

2

From your code it seems that you never clear the logList, this means that it appears the file is being appending because the logList still contains all of the old entries.

You'll need to clear the list between copies if you only want relevant entries to that copy, either before you start copying or after you finish writing the file.

This would be better as a separate method

    try
    {
        CopyAll(dirSource, dirTarget);

        SaveLog(target + @"\log.txt");

        ClearLog();
        //...
    }

    private void SaveLog(string filename)
    {
        if (logList.Count > 0)
        {
            FileStream fs = File.Open(target + @"\log.txt", FileMode.Create);
            using (StreamWriter sw = new StreamWriter(fs))
            {
                foreach (string error in logList)
                {
                    sw.WriteLine(error);
                }
            }
        }
    }
James
  • 9,774
  • 5
  • 34
  • 58
  • Thank you! Silly little thing to miss...appreciate it! – Matt Nov 26 '15 at 16:47
  • @JamesBarrass In general it's recommended to use log frameworks [as you said](http://stackoverflow.com/questions/33943285/overwrite-file-only-works-once#comment55646518_33943519) but for a small application, too keep things simple, `System.IO.File.WriteAllLines` or `System.IO.File.AppendAllLines` can be used [simply](http://stackoverflow.com/a/32515929/3110834). +1 :) – Reza Aghaei Nov 26 '15 at 17:08
  • Can someone give me some info about log frameworks, perhaps some frameworks you like to use yourselves or a simple one so I can get started learning about them? – Matt Nov 26 '15 at 17:15
  • @Matt If you want to learn more about log frameworks, you can take a look at [Log4Net](https://logging.apache.org/log4net/) or [NLog](http://nlog-project.org/) or [Enterprise Library Logging Application Block](https://msdn.microsoft.com/en-us/library/ff647240.aspx). But for a small application and until you gain required knowledge about using log framworks, you can keep using your own solution. Also you can make it more simple using `System.IO.File.WriteAllLines` or `System.IO.File.AppendAllLines`. – Reza Aghaei Nov 26 '15 at 17:46
  • Thanks will look into those frameworks as well as System.IO.File class – Matt Nov 27 '15 at 18:22
  • James, thank you for the suggestion on how to improve the code design and separating the methods..definitely makes sense to keep them separated like that – Matt Nov 27 '15 at 18:24
2

As @Reza Aghaei pointed out in comments, the problem is that you do not clear the logList.

The file gets created anew every time, but each time you click the Copy button, the loglist still contains the results of the previous copy action.

So you need to clear the list when starting a new copy:

private static void CopyAll(DirectoryInfo source, DirectoryInfo target)
{
    logList.Clear();

    // ...
Community
  • 1
  • 1
CodeCaster
  • 147,647
  • 23
  • 218
  • 272
  • Thank you! Such a silly little thing to miss...I wish I could give everyone the green checkmark. Only went with James above because I put the clear in the button, not the CopyAll method (but I assume it would still work)..thanks for the help! – Matt Nov 26 '15 at 16:46
  • @CodeCaster That's it +1 ;) – Reza Aghaei Nov 26 '15 at 16:47
  • @Matt the problem with James's approach is that [it breaks SOLID](https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)). (Well, the entire code does, but that's a different story). Say you want to display the log list on your screen and _not_ write it to a file, you'll have the same problem again. Clear it in the method that fills it. It's that method's responsibility. – CodeCaster Nov 26 '15 at 16:47
  • hmm..good point...Im kinda just a hobbyist at this point so Im not too familiar with SOLID..gonna read up on it a bit...but that makes sense. Thanks again! – Matt Nov 26 '15 at 16:50
  • @Matt no problem, you can accept the answer that helps _you_ the most. – CodeCaster Nov 26 '15 at 16:51
  • 1
    @CodeCaster intentionally. To record the log *since the last write to disk* but good point. This is why I like logging frameworks! – James Nov 26 '15 at 16:54
  • @CodeCaster if its not too much trouble, could you give me some tips on how I could improve my code design or maybe at least why my code isnt designed as best as possible/breaks SOLID? Maybe just some good resources to learn about SOLID design if you can. That would really be a big help. – Matt Nov 26 '15 at 17:13
  • @Matt that's a bit broad to expand on here, but check the wiki link in that comment. :) – CodeCaster Nov 26 '15 at 17:17
  • @CodeCaster will do. Thanks again! Happy holidays all! – Matt Nov 26 '15 at 17:30