File.ReadAllBytes Code Refactoring

Question

I came across this piece of code today:

public static byte[] ReadContentFromFile(String filePath)
{
    FileInfo fi = new FileInfo(filePath);
    long numBytes = fi.Length;
    byte[] buffer = null;
    if (numBytes > 0)
    {
        try
        {
            FileStream fs = new FileStream(filePath, FileMode.Open);
            BinaryReader br = new BinaryReader(fs);
            buffer = br.ReadBytes((int)numBytes);
            br.Close();
            fs.Close();
        }
        catch (Exception e)
        {
            System.Console.WriteLine(e.StackTrace);
        }
    }
    return buffer;
}

My first thought is to refactor it down to this:

public static byte[] ReadContentFromFile(String filePath)
{
    return File.ReadAllBytes(filePath);
}

System.IO.File.ReadAllBytes is documented as:

Opens a binary file, reads the contents of the file into a byte array, and then closes the file.

... but am I missing some key difference?

Answer 1

The original code returns a null reference if the file is empty, and won't throw an exception if it can't be read. Personally I think it's better to return an empty array, and to not swallow exceptions, but that's the difference between refactoring and redesigning I guess.

Oh, also, if the file length is changed between finding out the length and reading it, then the original code will read the original length. Again, I think the File.ReadAllBytes behaviour is better.

What do you want to happen if the file doesn't exist?

Answer 2

In this case, no you are not missing anything at all. from a file operation standpoint. Now you know that your lack of exception handling will change the behavior of the system.

It is a streamlined way of reading the bytes of a file.

NOTE: that if you need to set any custom options on the read, then you would need the long form.

Answer 3

That's basically the same method if you add the try {...} catch{...} block. The method name, ReadContentFromFile, further proves the point.

What a minute... isn't that something a unit test should tell?