1
\$\begingroup\$

I've recently made a test. There were four tasks. One of them was to improve following code:

public class FooFileProcessor
{
    public void ProcessFile(string filename)
    {
       Stream fileStream = File.OpenRead(filename);
       Console.WriteLine(ReadAllContent(fileStream));
       fileStream.Close();
    }

    public string ReadAllContent(Stream stream)
    {
       StreamReader streamReader = new StreamReader(stream);
       return streamReader.ReadToEnd();
    }
}

What I've suggested:

try
{ 
    using (StreamReader sr = new StreamReader("TestFile.txt"))
    {
       String line = sr.ReadToEnd();
       Console.WriteLine(line);
    }
}
catch (Exception e)        
{
   Console.WriteLine("The file could not be read:");
   Console.WriteLine(e.Message);
}

please, review my code and show all mistakes you've seen.

\$\endgroup\$
3
  • \$\begingroup\$ Shortening the code is not always an improvement. \$\endgroup\$ Commented Apr 7, 2016 at 12:14
  • \$\begingroup\$ @denis is it okay in my case? \$\endgroup\$ Commented Apr 7, 2016 at 14:04
  • 1
    \$\begingroup\$ I would say @Corey's approach is probably the best in this case. \$\endgroup\$ Commented Apr 7, 2016 at 15:09

2 Answers 2

Reset to default
4
\$\begingroup\$

The first thing that springs to mind is that it would probably be better to not create the Stream and StreamReader instances when it can be avoided simply by using the File.ReadAllText method to accomplish the same goal:

public void ProcessFile(string filename)
{
   Console.WriteLine(File.ReadAllText(filename));
}

In other situations you might need to avoid this, especially for very large files. For small files and simple operations the File.Read... methods should suffice.

Main advantage is that you relegate the object management issues to the framework, reducing the complexity of your code. This should result in fewer bugs and greater readability.

For larger files, especially where you are wanting to process individual lines (a common use case) it is often useful to use the File.ReadLines method to process the content. This should read the lines as needed with some buffering, reducing memory consumption while maintaining code simplicity.

On the question of exception handling... I prefer to only put exception handling in when it is required by the specific situation, and I didn't see a specific need here. If the idea of an exception breaking out of this code concerns you then by all means add some in. Personally I'd prefer to have the exception bubble out than have to write all the extra code in here to not only handle the exception but also change what we do with a null return, etc.

In general I don't like the idea of always trying to catch exceptions as soon as possible. I prefer to catch as soon as I need to and no sooner.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Exactly what I would have expected too. I'd say that the exception handling the OP added was a worthwhile addition too. \$\endgroup\$ Commented Apr 5, 2016 at 8:30
  • \$\begingroup\$ @RobH so my code okay?) \$\endgroup\$ Commented Apr 5, 2016 at 8:44
  • 1
    \$\begingroup\$ In regard to the exception handling - I'd say this is the latest you can reasonably catch the exception as the method is attempting to output to the user. If it can't, the user should know something happened. I agree wholeheartedly with your comments though. \$\endgroup\$ Commented Apr 5, 2016 at 12:50
  • 1
    \$\begingroup\$ Remember to wrap this in a try-catch block. File.ReadAllText has 9 exceptions that it can throw, of which at least 3 are exogenous exceptions which must be caught at some point. \$\endgroup\$ Commented Apr 5, 2016 at 12:52
1
\$\begingroup\$

I would say your changes are too invasive to only consider them as improving the code. I have to admit I'm not sure what is expected here, though. You also didn't say what method your code belongs to. Anyway, here is my approach:

public void ProcessFile(string filename)
{
    Stream fileStream = null;

    try
    {
        fileStream = File.OpenRead(filename);
        Console.WriteLine(ReadAllContent(fileStream));
    }
    finally
    {
        if (fileStream != null)
        {
            fileStream.Close();
        }
    }
}

public string ReadAllContent(Stream stream)
{
    using (var streamReader = new StreamReader(stream))
    {
        return streamReader.ReadToEnd();
    }
}

Alternatively to try/finally, you can use a using construct similar to the one in ReadAllContent method. I decided to go with try/finally to include the call to Close, which is present in the original code snippet. Stream.Close calls Dispose in its implementation, so there is no real difference in behaviour.

public virtual void Close()
{
  this.Dispose(true);
  GC.SuppressFinalize((object) this);
}

The if (fileStream != null) check is necessary in case an exception is thrown when trying to open the filename file for reading.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.