File processor for text and HTML I recently took a test. One of the questions was to find problems in the following code and suggest refactoring which can provide adding new formats of data:


public class FileProcessor
{
    enum FileType
    {
        Html, Text
    }

    public void ProcessFile(string fileName)
    {
        StreamReader fileStream = new StreamReader(File.OpenRead(fileName));
        string fileContent = fileStream.ReadToEnd();

        if (fileContent.IndexOf("&lt;html") != -1)
            ProcessHtmlFile(fileContent);
        else ProcessTextFile(fileContent);

        ProcessFile(fileContent,fileContent.IndexOf("&lt;html")!= -1?
                                     FileType.Html:FileType.Text);
        fileStream.Close();

    }

    private void ProcessFile(string content,  FileType fileType)
    {

        switch(fileType)
        {
            case FileType.Html:
                ProcessHtmlFile(content);
                break;
            case FileType.Text:
                ProcessTextFile(content);
                break;
            default:
                throw new Exception("Unknown file format");
        }
    }

    private void ProcessHtmlFile(string content){...}

    private void ProcessTextFile(string content){...}

}



What I've suggested:

I've suggested to use open closed principle and my implementation is:

public class FileProcessor
{
   public void ProcessFile(string fileName, IFile file)
   {
      StreamReader fileStream = new StreamReader(File.OpenRead(fileName));
      string fileContent = fileStream.ReadToEnd();

      file.ProcessFooFile(fileContent);
      fileStream.Close();
   }
}

public interface IFile
{
    void ProcessFooFile(string fileContent);
}

public class HtmlFile : IFile
{
   public void ProcessFooFile(string fileContent)
   {
      //Process HTML File
   }
}

public class TextFile : IFile
{
   public void ProcessFooFile(string fileContent)
   {
      //Process Text File
   }
}


My questions are:


Is it appropriate to use open closed principle in this case?
Is my implementation correct? Especially, please, see that I deleted a row ProcessFile(fileContent,fileContent.IndexOf("&lt;html")!= -1?FileType.Html:FileType.Text); from public void ProcessFile(string fileName) as there is no need in the row.
Is there some drawbacks in my implementation. Please, provide improvements.
Ad 1. Yes, it's reasonable to refactor it the way you did. It's consistent with Single Responsibility Principle, and as such it makes it easy to eg. add some new formats without having to meddle with the base routine.

Ad 2. It's questionable in some places.

a) No exception handling. You may prefer it to be caller's responsibility, but at the very least you should ensure that the file stream always gets closed, even if an exception is thrown (with a try-finally or using).

b) The names IFile and its derivatives are misleading. It's a IFileProcessor (HtmlFileProcessor etc.). It's obvious that it doesn't represent a file itself, it represents a way of dealing with a file whose contents are passed from outside.

Ad 3. Your new implementation lacks the part that associated appropriate file processing method with a given FileType. This could be implemented with a FileProcessorFactory class taking a FileType instance and returning an IFileProcessor instance. Calling code doesn't need to know whether it's HtmlFileProcessor etc., that's the point: such nuts and bolts are something left for the factory to worry about, and caller is only concerned whether the factory returns something that matches the interface.

EDIT:

Here's a suggested improvement.

public class FileProcessor
{
    public void ProcessFile(string fileName, IFileProcessorBehavior file)
    {
        // using ensures we won't leave the method without closing the fileStream
        // even if an exception is thrown. this prevents resource leaks.
        using (var fileStream = new StreamReader(File.OpenRead(fileName))) 
        {
            string fileContent = fileStream.ReadToEnd();
            file.ProcessFooFile(fileContent);
        }
    }
}

// Behavior, or Strategy. 
// we need to differentiate file processor the general facade
// from format-specific file processor.
// in any case, this is not a "file", it's a way of dealing with a file.
// if I knew more about the context of this code, 
// I could probably come up with a more natural naming scheme
public interface IFileProcessorBehavior
{
    void ProcessFooFile(string fileContent);
}

public class FileBehaviorFactory
{
    public IFileProcessorBehavior Create(FileType fileType)
    {
        switch(fileType)
        {
            case FileType.Html:
                return new HtmlBehavior();
            case FileType.Text:
                return new TextFileBehavior();
            default:
                // don't use the most generic Exception - use a meaningful exception type
                throw new NotSupportedException("Unknown file format: " + fileType); 
                // ^ and quote the value that was not recognized -
                // it will help to track the cause of an error
        }
    }
}

public class HtmlBehavior : IFileProcessorBehavior
{
    public void ProcessFooFile(string fileContent)
    {
        // ...
    }
}

public class TextFileBehavior : IFileProcessorBehavior
{
    public void ProcessFooFile(string fileContent)
    {
        // ...
    }
}


And now you would just:

var factory = new FileBehaviorFactory();
IFileProcessorBehavior behavior = factory.Create(fileType);
// at this point we don't know and we don't care of what type this behavior thing is
var processor = new FileProcessor("some file", behavior); 


Thanks to this pattern any new formats can be added as separate classes (supporting the interface), so the system is open for extension, and they can be registered within the factory. The base routine (FileProcessor) is closed, which is what we want. The factory only carries one responsibility, which is to match any given file type to respective behavior, or file processing strategy. This means there is only one ever one reason to modify the factory: when this logic needs to be enhanced, or modified.