views:

847

answers:

7

Often I find myself interacting with files in some way but after writing the code I'm always uncertain how rubust it actually is. The problem is that I'm not entirely sure how file related operations can fail and, therefore, the best way to handle expections.

The simple solution would seem to be just to catch any IOExceptions thrown by the code and give the user a "Inaccessible file" error message but is it possible to get a bit more fine-grained error messages. Is there a way to determine the difference between such errors as a file being locked by another program and the data being unreadable due to a hardware error?

Given the following C# code, how would you handle errors in a user friendly (as informative as possible) way?

public class IO
{
   public List<string> ReadFile(string path)
   {
      FileInfo file = new FileInfo(path);

      if (!file.Exists)
      {
         throw new FileNotFoundException();
      }

      StreamReader reader = file.OpenText();
      List<string> text = new List<string>();

      while (!reader.EndOfStream)
      {
         text.Add(reader.ReadLine());
      }

      reader.Close();
      reader.Dispose();
      return text;
   }

   public void WriteFile(List<string> text, string path)
   {
      FileInfo file = new FileInfo(path);

      if (!file.Exists)
      {
         throw new FileNotFoundException();
      }

      StreamWriter writer = file.CreateText();

      foreach(string line in text)
      {
         writer.WriteLine(line);
      }

      writer.Flush();
      writer.Close();
      writer.Dispose();
   }
}
A: 

I would try and check for the file.Exists before calling your read/write and respond to the user there, instead of creating the overhead of raising an error and catching it later since the check is so easy to make. I do understand the need for raising errors but in this particulary case a simple check imho would be a better solution. What i mean is add one more method to check if the file exists.

Also if you do check beforehand if the file exits, you know that something else is blocking it if you can't write to it. Also you can catch multiple exceptions the first one to match will be caught - but you probably know this...

Per Hornshøj-Schierbeck
+4  A: 

The first thing you should change are your calls to StreamWriter and StreamReader to wrap them in a using statement, like this:

using (StreamReader reader = file.OpenText())
{
   List<string> text = new List<string>();
   while (!reader.EndOfStream)
   {
      text.Add(reader.ReadLine());
   }
}

This will take care of calling Close and Dispose for you and will actually wrap it in a try/finally block so the actual compiled code looks like this:

StreamReader reader = file.OpenText();
try
{
   List<string> text = new List<string>();
   while (!reader.EndOfStream)
   {
      text.Add(reader.ReadLine());
   }
}
finally
{
   if (reader != null)
      ((IDisposable)reader).Dispose();
}

The benefit here is that you ensure the stream gets closed even if an exception occurs.

As far as any more explicit exception handling, it really depends on what you want to happen. In your example you explicitly test if the file exists and throw a FileNotFoundException which may be enough for your users but it may not.

Scott Dorman
I actually knew about the using directive but had forgotten it. However, I didn't know that it added a try-catch as well. Cool!
Morten Christiansen
Well, it actually adds a try/finally rather than a try/catch. That's how it is able to ensure Dispose is called even when an exception occurs.
Scott Dorman
+1  A: 
  • Skip the File.Exists(); either handle it elsewhere or let CreateText()/OpenText() raise it.
  • The end-user usually only cares if it succeeds or not. If it fails, just say so, he don't want details.

I haven't found a built-in way to get details about what and why something failed in .NET, but if you go native with CreateFile you have thousands of error-codes that can tell you what went wrong.

Rune
A: 

I would use the using statement to simplify closing the file. See MSDN the C# using statement

From MSDN:

  using (TextWriter w = File.CreateText("log.txt")) {
     w.WriteLine("This is line one");
     w.WriteLine("This is line two");
  }
  using (TextReader r = File.OpenText("log.txt")) {
     string s;
     while ((s = r.ReadLine()) != null) {
        Console.WriteLine(s);
     }
  }
Brian
+1  A: 

I don't see the point in checking for existence of a file and throwing a FileNotFoundException with no message. The framework will throw the FileNotFoundException itself, with a message.

Another problem with your example is that you should be using the try/finally pattern or the using statement to ensure your disposable classes are properly disposed even when there is an exception.

I would do this something like the following, catch any exception outside the method, and display the exception's message :

public IList<string> ReadFile(string path)
{
    List<string> text = new List<string>();
    using(StreamReader reader = new StreamReader(path))
    {
      while (!reader.EndOfStream)      
      {         
         text.Add(reader.ReadLine());      
      }
    }
    return text;
}
Joe
A: 

Perhaps this is not what you are looking for, but reconsider the kind you are using exception handling. At first exception handling should not be treated to be "user-friendly", at least as long as you think of a programmer as user.

A sum-up for that may be the following article http://goit-postal.blogspot.com/2007/03/brief-introduction-to-exception.html .

Georgi
By user friendly I mean that I would like to be able to give the user an idea of which type of error ocurred.
Morten Christiansen
By "perhaps not what you are looking for" I meant not the direct answer - but the procedures to get your exception / io - handling more user-friendly. This is not a short shot at all but needs more reflection about how to program in general.
Georgi
+2  A: 

...but is it possible to get a bit more fine-grained error messages.

Yes. Go ahead and catch IOException, and use the Exception.ToString() method to get a relatively relevant error message to display. Note that the exceptions generated by the .NET Framework will supply these useful strings, but if you are going to throw your own exception, you must remember to plug in that string into the Exception's constructor, like:

throw new FileNotFoundException("File not found");

Also, absolutely, as per Scott Dorman, use that using statement. The thing to notice, though, is that the using statement doesn't actually catch anything, which is the way it ought to be. Your test to see if the file exists, for instance, will introduce a race condition that may be rather vexing. It doesn't really do you any good to have it in there. So, now, for the reader we have:

try {  
    using (StreamReader reader = file.OpenText()) {  
        // Your processing code here  
    }  
} catch (IOException e) {  
    UI.AlertUserSomehow(e.ToString());  
}

In short, for basic file operations:
1. Use using
2, Wrap the using statement or function in a try/catch that catches IOException
3. Use Exception.ToString() in your catch to get a useful error message
4. Don't try to detect exceptional file issues yourself. Let .NET do the throwing for you.

Dustman