Using a factory class for TextWriter than can be stubbed/mocked

This is my factory class:

/// 
/// Inject TextWriterFactory into the class you need to test
/// In the unit test, use SetWriter() to set a different TextWriter (ex: StringWriter)
/// Add extra overloads for Create method if needed
/// 
public class TextWriterFactory {
    private TextWriter _writer = null;
    public TextWriter Create(string path, bool append) {
        if (_writer != null)
            return _writer;

        return new StreamWriter(path, append);
    }

    public void SetWriter(TextWriter writer) {
        _writer = writer;
    }
}

This is how it's used in my test methods:

[TestMethod]
public void Test() {
    ...
    var textWriterFactory = new TextWriterFactory();
    textWriterFactory.SetWriter(new StringWriter());
    // inject into the class I am testing
}

And in my production code:

public void DoSomething() {
    using (TextWriter writer = _textWriterFactory.Create(template.LocalTempFilePath, false))
        writer.Write(template.FileContent);
    ...
}

Does that seem ok to you or is there a better way? I plan to use this everywhere in my code that needs to be tested. I'm using moq framework and SystemWrapper for other mscorlib methods I need to stub/mock.

In dependency injection terms, this factory class has a dependency upon a TextWriter instance. I would much prefer to see that dependency be constructor-injected:

private readonly TextWriter _writer;

public TextWriterFactory()
    : this(new StringWriter())
{ }

public TextWriterFactory(TextWriter writer)
{
    _writer = writer;
}

That way you get rid of the hard-coded default StreamWriter. But there are a number of issues with this approach.

• TextWriter being an abstract class is nice, but this abstract class implements the IDisposable interface, which makes it a leaky abstraction to use as a dependency.

• The class is still tightly coupled with StringWriter, through the new StringWriter() call in the default constructor. This is actually a DI anti-pattern that Mark Seemann calls bastard injection in Dependency Injection in .NET.

-
You're dealing with an IDisposable here; I'd expect this test code to throw a rather surprising ObjectDisposedException:

var factory = new TextWriterFactory();
factory.SetWriter(new StringWriter());

using (var writer = factory.Create("C:\foo\bar.txt", false))
{
    writer.Write("append=false");
}

using (var writer = factory.Create("C:\foo\bar.txt", true))
{
    writer.Write("append=true");
}

Let's take a step back here.

Your production code has a dependency that hits the file system.

You need an abstraction to wrap it with, and be able to inject an implementation that doesn't hit the file system, so you can test your code; the simplest abstraction remains an interface - it doesn't tie you up with a base class with embedded behavior that you don't want your tests to have to care for.. or worry about.

I would simply wrap the writer with an interface, with the bare minimum members you need for it to work with your code:

public interface IWriter
{
    void Write(string value);
}

Then I would compose an implementation for the production code:

public class Writer : IWriter, IDisposable
{
    private readonly StreamWriter _writer;

    public Writer(string path, bool append)
    {
        _writer = new StreamWriter(path, append);
    }

    public void Write(string value)
    {
        _writer.Write(value);
    }

    public void Dispose()
    {
        _writer.Dispose();
    }
}

Now you can constructor-inject an IWriter instead of a "factory", and your tests can inject a mock implementation, and the mock can know if the Write method was called with the expected value parameter.

Note that I could have done this too:

public interface IWriter : IDisposable
{
    void Write(string value);
}

But that would have been a leaky abstraction - the fact that we're encapsulating a TextWriter that implements IDisposable shouldn't be leaking into the abstraction, because the TextWriter isn't the abstraction, it's an implementation detail: you could just as well write an implementation that doesn't use a StreamWriter and isn't disposable - for example:

public class FileWriter : IWriter
{
    private readonly string _path;
    private readonly bool _append;

    public Writer(string path, bool append)
    {
        _path = path;
        _append = append;
    }

    public void Write(string value)
    {
        if (_append)
        {
            File.AppendAllText(_path, value);
        }
        else
        {
            File.WriteAllText(_path, value);
        }
    }
}

Now your production code can look like this:

public void DoSomething() 
{
    _writer.Write(template.FileContent);
}

public void Dispose()
{
    var writer = _writer as IDisposable;
    if (writer != null)
    {
        writer.Dispose();
    }
}

Now, regarding the usability of this code, there's still an itchy spot: I put the string path, bool append parameters in the constructor, because you had them in your Create "factory method".

Ideally you would make them parameters of the Write method: injecting primitives through the constructor is pretty much always a smell.

Also note that C# conventions place the { opening brace on the next line.

StackExchange Code Review Q#61093, answer score: 12