5
\$\begingroup\$

Context

I'm working on a new Enterprise Library at my company and I am implementing some code that will be used to read files into memory. Since this is an enterprise library to be shared across the organization, the size of the file to be read in is unknown. I chose to manually read the contents of the files into memory (using a buffer) instead of File.ReadAllText in order to have more control over the read process. For now, the buffer size is fixed at 512 bytes, but may be configurable in the future to allow a given use-case to have a larger or smaller buffer size. I have come up with the following code (which appears to work well):

Code

/// <summary>
/// Class that provides access to the file system by wrapping system level file IO interactions.
/// </summary>
public class FileSystemProvider : IFileSystemProvider {

    /// <summary>
    /// Initializes a new instance of the <see cref="FileSystemProvider"/> class.
    /// </summary>
    public FileSystemProvider() {
    } // end default constructor

    /// <summary>
    /// Returns a value indicating whether or not the specified file exists.
    /// </summary>
    /// <param name="filePath">string:  The fully qualified path to the file.</param>
    /// <returns>true if the specified file is found; false otherwise</returns>
    public virtual bool FileExists(string filePath) {
        bool returnValue = false;
        if (this.IsValidPath(filePath)) {
            returnValue = File.Exists(filePath);
        }

        return returnValue;
    } // end function FileExists

    /// <summary>
    /// Returns a value indicating whether or not the given path is valid.  This method is provided as a means to keep the code
    /// DRY by providing a common place to verify the contents of a string prior to performing any file IO operations.  This
    /// method can / should be overridden by a derived type to perform any additional checks (regex, length, etc...).  The
    /// default implementation just checks for a null or whitespace-trimmed string.
    /// </summary>
    /// <param name="path">string:  The path to check.</param>
    /// <returns>true if the path is not null or an empty, whitespace-trimmed string; false otherwise</returns>
    protected virtual bool IsValidPath(string path) {
        bool returnValue = false;
        if (!string.IsNullOrWhiteSpace(path)) {
            returnValue = true;
        }

        return returnValue;
    } // end function IsValidPath

    /// <summary>
    /// Reads the contents of the file using the specified encoding and returns the string-based content.
    /// </summary>
    /// <param name="filePath">string:  The fully qualified path to the file.</param>
    /// <param name="fileEncoding"><see cref="System.Text.Encoding"/>: The encoding used to convert the file binary to a readable string.</param>
    /// <returns>The contents of the specified file as a string.</returns>
    public virtual string ReadFileContents(string filePath, Encoding fileEncoding) {
        string returnValue = string.Empty;

        if (this.FileExists(filePath)) {
            // Stream the contents of the file to reduce the memory load for large files
            StringBuilder output = new StringBuilder();
            byte[] bufferBlock = new byte[512];

            using (FileStream fs = new FileStream(filePath, FileMode.Open)) {
                if (fs.Length > bufferBlock.Length) {
                    long remainingBytes = fs.Length;
                    while (remainingBytes > 0) {
                        if (remainingBytes >= bufferBlock.Length) {
                            fs.Read(bufferBlock, 0, bufferBlock.Length);
                            output.Append(fileEncoding.GetString(bufferBlock).Trim());
                        } else {
                            Array.Clear(bufferBlock, 0, bufferBlock.Length);
                            fs.Read(bufferBlock, 0, (int)remainingBytes); // We can safely cast the long here because we know it's value is less than 512
                            output.Append(fileEncoding.GetString(bufferBlock, 0, (int)remainingBytes).Trim());
                        }

                        remainingBytes -= bufferBlock.Length;
                    }
                } else {
                    // The total file length is smaller than our buffer size, so read it all in at once, trimming the excess buffer space
                    fs.Read(bufferBlock, 0, (int)fs.Length);
                    output.Append(fileEncoding.GetString(bufferBlock, 0, (int)fs.Length).Trim());
                }
            }

            returnValue = output.ToString();
        }

        return returnValue;
    } // end function ReadFileContents

    /// <summary>
    /// Reads the raw contents of the file.
    /// </summary>
    /// <param name="filePath">string:  The fully qualified path to the file.</param>
    /// <returns>An array of the raw byte information stored in the file.</returns>
    public virtual byte[] ReadFileContents(string filePath) {
        byte[] returnValue = null;
        if (this.FileExists(filePath)) {
            byte[] buffer = new byte[512];
            returnValue = new byte[0];
            using (FileStream fs = new FileStream(filePath, FileMode.Open)) {
                if (fs.Length > buffer.Length) {
                    long remainingBytes = fs.Length;
                    while (remainingBytes > 0) {
                        if (remainingBytes >= buffer.Length) {
                            fs.Read(buffer, 0, buffer.Length);
                            int oldLength = returnValue.Length;
                            int newLength = returnValue.Length + buffer.Length;
                            Array.Resize<byte>(ref returnValue, newLength);
                            Array.Copy(buffer, 0, returnValue, oldLength, buffer.Length);
                        } else {
                            Array.Clear(buffer, 0, buffer.Length);
                            fs.Read(buffer, 0, (int)remainingBytes);
                            int oldLength = returnValue.Length;
                            int newLength = returnValue.Length + (int)remainingBytes;
                            Array.Resize<byte>(ref returnValue, newLength);
                            Array.Copy(buffer, 0, returnValue, oldLength, (int)remainingBytes);
                        }

                        remainingBytes -= buffer.Length;
                    }
                } else {
                    fs.Read(buffer, 0, (int)fs.Length);
                    Array.Resize<byte>(ref returnValue, (int)fs.Length);
                    Array.Copy(buffer, 0, returnValue, 0, (int)fs.Length);
                }
            }
        } 

        return returnValue;
    } // end function ReadFileContents

Review Points

I'm interested in a general code review, but I'm specifically looking for input regarding:

  • How will this code handle very large files (currently tested up to a file length of 5098 kB)?
  • Is using the Array.Resize<> method in the byte[] return overload efficient, or is there a better way?
  • Is there a way to refactor to improve the readability and / or simplify the code?
\$\endgroup\$
  • \$\begingroup\$ Is this a text file or a binary file? If I am not wrong you are interpreting contents of any binary file as text - which you shouldn't be doing since you will loose data... see my answer. \$\endgroup\$ – user59884 Dec 11 '14 at 9:40
  • \$\begingroup\$ I don't get why you're not using File.ReadAllBytes and only replace the implementation when there is clear performance evidence that this is an improvement. \$\endgroup\$ – CodesInChaos Dec 11 '14 at 12:47
  • \$\begingroup\$ "will this code handle very large files" -- No code that returns the whole file in a byte array can handle files larger than 2GiB since the current .NET implementation limits array to that size even for a 64 bit process. The GC will probably be unhappy before you reach that size. \$\endgroup\$ – CodesInChaos Dec 11 '14 at 12:49
  • \$\begingroup\$ The file may be binary or it may be text, that's why I provide the overload to make it convenient when reading text -based files. @CodesInChaos, don't know if there is a reason. Would you be willing to submit an answer with your recommendation? \$\endgroup\$ – xDaevax Dec 11 '14 at 13:42
  • 1
    \$\begingroup\$ @xDaevax: because the method which returns string will not work correctly if file is binary \$\endgroup\$ – user59884 Dec 11 '14 at 13:50
7
\$\begingroup\$

Guard Clauses

You have code like:

public virtual string ReadFileContents(string filePath, Encoding fileEncoding) {
    string returnValue = string.Empty;

    if (this.FileExists(filePath)) {
        ... do stuff
        returnValue = .....
    }
    return returnValue;
}

This would be better written as a guard clause:

public virtual string ReadFileContents(string filePath, Encoding fileEncoding) {

    if (!this.FileExists(filePath)) {
        return string.Empty;
    }

    ... do stuff
    return ....;
}

Performance

Your concepts of 'large' and 'very large', are unconventional. I would consider 'large' to be in the > 100MiB ballpark, and very large to be > 4GiB (more than 32-bit size).

This has impacted the features you consider to be performance-enhancing. All file-systems I know of use at least a 512-byte extent, with the extends merged in to at least 4KiB blocks. If you are buffering data, I would recommend at least a 4KiB buffer. I have done similar things in the past, and I now typically use a 1MiB buffer to get queued reads happening.

I would thus have a buffer like:

var bufferSize = Math.Min(1024 * 1024, fs.Length)
byte[] bufferBlock = new byte[bufferSize];

That will set a buffer that can read all, or big chunks of the file.

If you do it that way, you can also remove the code path for files that are smaller than the buffer, they become irrelevant.

Byte[] method: byte[] ReadFileContents(string filePath)

This method is horrible overkill. Since you have to return all the bytes anyway, you may as well just allocate a single large buffer for the file size, populate it, and return it.

if (fs.Length > (long)Int32.MaxValue)
{
    ... throw appropriate exception
}
byte[] returnValue = new byte[(int32)fs.Length];
\$\endgroup\$
  • \$\begingroup\$ Wonderful suggestion for the buffer size. That certainly would make it much cleaner. I've not really used the Guard clause pattern (except to throw exceptions), but I think you're right that it makes sense here to terminate the function early. \$\endgroup\$ – xDaevax Dec 10 '14 at 17:27
4
\$\begingroup\$

Questionable methods

public virtual bool FileExists(string filePath) {
    bool returnValue = false;
    if (this.IsValidPath(filePath)) {
        returnValue = File.Exists(filePath);
    }

    return returnValue;
}

I'm not really sure of your intent here? What is wrong with the existing File.Exists method? IsValidPath isn't actually checking for a valid path, it's just checking !string.IsNullOrWhiteSpace(). You can remove both FileExists and IsValidPath here and replace calls to FileExists with File.Exists()

In any case, if you were to keep them, for IsValidPath this would be much more appropriate and readable:

protected virtual bool IsValidPath(string path) 
{
    return !string.IsNullOrWhiteSpace(path)
}

Styling

Usually in C# we have curly braces on new lines:

protected virtual bool IsValidPath(string path) { 

Should be:

protected virtual bool IsValidPath(string path)
{

Your concerns

Will fill in later...

\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback! The primary reason for those methods (and this whole class frankly) is to wrap the Framework methods so that code that needs to access the file system can have a mocked IFileSystemProvider injected while testing. Additionally, there may be some applications that do RegEx validation on the path, so I wanted to create an abstraction (IsValidPath) that all methods use, so RegEx checking can be done in a single place instead of each method having to call File.Exists and !string.IsNullorWhiteSpace. I look forward to your additional feedback! \$\endgroup\$ – xDaevax Dec 10 '14 at 17:23
  • \$\begingroup\$ later... it's been a while now. Will you still fill in later? \$\endgroup\$ – Vogel612 Dec 15 '14 at 17:16
3
\$\begingroup\$
  • How will this code handle very large files (currently tested up to a file length of 5098)?

Already answered by rolfl.

  • Is using the Array.Resize<> method in the byte[] return overload efficient, or is there a better way?
  • Is there a way to refactor to improve the readability and / or simplify the code?

As you have a filestream and a defined buffer size you can simply copy the content of the filestream to a memory stream. You can then turn this memory stream into a byte array.

After implementing the guard conditions and buffersize like rolfl mentioned your former methods will look like

public virtual byte[] ReadFileContents(string filePath)
{
    if (!this.FileExists(filePath)) { return null; }
    using (FileStream fs = new FileStream(filePath, FileMode.Open))
    {
        int bufferSize = (int)Math.Min(1024 * 1024, fs.Length);
        using (MemoryStream ms = new MemoryStream(bufferSize))
        {
            fs.CopyTo(ms, bufferSize);
            return ms.ToArray();
        }
    }

}
public virtual string ReadFileContents(string filePath, Encoding fileEncoding)
{
    string returnValue = string.Empty;
    byte[] content = ReadFileContents(filePath);

    if (content == null) { return String.Empty; }

    return fileEncoding.GetString(content);
}

Based of xDaevax comment

The Stream.CopyTo() method is calling the InternalCopyTo() method which in turn is doing this

byte[] buffer = new byte[bufferSize];
int read;
while ((read = Read(buffer, 0, buffer.Length)) != 0)
    destination.Write(buffer, 0, read);

After discussing these with rolfl I will change the former implementation to

private const int defaultBlockSize = 81920;
public virtual byte[] ReadFileContents(string filePath)
{
    return InternalReadFileContent(filePath, defaultBlockSize);
}

private byte[] ReadFileContents(string filePath, int blockSize)
{
    return InternalReadFileContent(filePath, blockSize);
}

public virtual byte[] InternalReadFileContent(string filePath, int blockSize)
{
    if (!this.FileExists(filePath)) { return null; }

    using (FileStream fs = new FileStream(filePath, FileMode.Open))
    {
        byte[] content = new byte[fs.Length];
        using (MemoryStream ms = new MemoryStream(content))
        {
            fs.CopyTo(ms, blockSize);
            return content;
        }
    }
}  

Now

  • the MemoryStream is initialized with an array which doesn't need to be resized
  • ToArray() which creates a copy of the internal buffer won't be called omitting the copy
  • the default buffersize of the Stream.CopyTo(Stream) is used if the overloaded ReadFileContents(String) is called.
  • if one want to use a different block size he/she can still do it
\$\endgroup\$
  • \$\begingroup\$ :He is interpreting binary files as text which is wrong. stackoverflow.com/questions/10353913/… \$\endgroup\$ – user59884 Dec 11 '14 at 8:05
  • \$\begingroup\$ @giorgim why don't you make an answer out of this ? \$\endgroup\$ – Heslacher Dec 11 '14 at 8:09
  • \$\begingroup\$ ok I did it .. you can also have a look \$\endgroup\$ – user59884 Dec 11 '14 at 8:16
  • \$\begingroup\$ Maybe I'm misreading, but it seems like this code would only read the first 4k, but ignore anything larger. \$\endgroup\$ – xDaevax Dec 11 '14 at 15:30
  • \$\begingroup\$ @xDaevax See stackoverflow.com/a/1933764/2655508 \$\endgroup\$ – Heslacher Dec 11 '14 at 16:13
3
\$\begingroup\$

At a glance I see several bugs:

  • You're not checking the return value of Stream.Read. It can be smaller than the length you passed in, but you're assuming it's equal.
  • The calls to Trim arbitrarily removes whitespace and nullbytes. I guess this is a workaround for not checking the return value Stream.Read.
  • fileEncoding.GetString will not work correctly if a buffer boundary falls in the middle of a codepoint that has been encoded as several bytes.

Some additional issues:

  • I don't see the point of most of your methods. ReadFileContents can be implemented by calling File.ReadAllText and File.ReadAllBytes. Simpler and less buggy. Even if you don't like File.ReadAllText, a TextReader will save you a lot of pain.
  • Why use naming different from the methods of File? Using familiar names makes it easier to learn your library as somebody who is already familiar with the BCL.
  • You're opening the file for writing when you only read. It's common to only have permission to read a file, not write it. Your method will fail unnecessarily for those.
  • I don't think silently returning nothing if the file doesn't exist is a good idea. That will usually turn a clear exception into a null reference exception. For the rare cases where a caller wants to handle absent files without an error, leave that to the caller.
\$\endgroup\$
1
\$\begingroup\$

I think you are getting it wrong. You can not interpret binary files as text files. What I mean is here:

fileEncoding.GetString(bufferBlock)

As I understood bufferBlock can contain contents of a binary file. Then you are calling a method which will try to interpret these bytes as some text encoded in some encoding. But this might not be the case, since the file was not a text file to begin with. More details here: https://stackoverflow.com/questions/10353913/streamreader-vs-binaryreader/

\$\endgroup\$
  • 1
    \$\begingroup\$ Now I understand what you have meant in the comments. Basically a file containing text is also just containing bytes. One needs to be sure to pass a "text file" to the method which returns a String. It is the same with e.g notepad. You can open a exe but you won't do it, because you know you won't get a meaningful result. \$\endgroup\$ – Heslacher Dec 11 '14 at 8:37
  • \$\begingroup\$ @Heslacher:Yes something like this. I mean you can still convert binary file to string but for that you would need to convert each byte to a string separately. The method OP is using-fileEncoding.GetString(bufferBlock)- is different I think it just thinks the bytes bufferBlock encode a text say in some encoding, whereas in reality it doesn't. The answer (and comments) in the link I mentioned explains that. \$\endgroup\$ – user59884 Dec 11 '14 at 9:00

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.