This test is not good as we can't test the ExtractFullName by itself - we're testing that CsvDocumentReader.GetContent works as well. In addition, we're depend on external file because this is what CsvDocumentReader.GetContent expects to receive.

 

 

Now CsvDataExtrator can get IDocumentReader in its constructor and use it in ExtractFullName. We could mock the interface and determine the result we want to get from the mocked object. Here is the refactored version:

public interface IDocumentReader
{
   string GetContent(string documentPath);
}

public class CsvDataExtractor
{
   private string _documentPath;
   private IDocumentReader _reader;

   public CsvDataExtractor(IDocumentReader reader, string documentPath)
   {
      _documentPath = document;
      _reader = reader;
   }

   public string ExtractFullName()
   {
      string content = _reader.GetContent(_documentPath);
      string fullName = // extract full name logic
      return fullName;
   }
}

[Test]
public void ExtractFullName_DocumentHasFirstNameAndLastNameOnly()
{
   DocumentReaderStub reader = new DocumentReaderStub();
   reader.ContentToReturn = "oren,ellenbogen";

   CsvDataExtractor extractor = new CsvDataExtractor(reader, "not important document path");

   string result = extractor.ExtractFullName();

   Assert.AreEqual("ellenbogen", result);
}

internal class DocumentReaderStub : IDocumentReader
{
   public string ContentToReturn;
   public string GetContent(string documentPath) { return ContentToReturn; }
}

 

Pros: (1) We can use mocking framework (Rhino Mocks for example is a great choice, and I'm not getting payed for it. I swear) to create stubs\mocks really fast and almost with no code. (2) Static classes and static members can not be easily tested, so we can save a few painful minutes\hours to the our teammates by refactoring now. (3) This one relates to reason 1 - If we need to mock several methods of our static class CsvDocumentReader, this is a better solution as we can achieve it easily with mocking framework.
Cons: (1) It's not always possible to refactor the original static class. For example, we can't change Microsoft's Guid static class to control NewGuid() method, assuming that we need to mock it.

 

We can create an interface that expose all the functionality of the static class. Then all we need to do is wrap it with a simple class that will delegate the calls to the static class:

// the interface should look exactly like the static class we want to wrap !
public interface IDocumentReader
{
   string GetContent(string documentPath);
}

// This class will simply delegate calls to the static class
public class CsvDocumentReaderWrapper : IDocumentReader
{
    public string GetContent(string documentPath)
    {
        return CsvDocumentReader.GetContent(documentPath); // call the original static class
    }
}

The CsvDataExtractor implementation and the tests are exactly the same as option 1.

 

Pros: You can enjoy all the Pros of option 1.

Cons: (1) Wrapping is costly performance and especially in maintenance. In addition, you suffer from all of the Cons of option 1.

 

public class CsvDataExtractor
{
   private string _documentPath;

   public CsvDataExtractor(string documentPath)
   {
      _documentPath = document;
   }

   public string ExtractFullName()
   {
      string content = GetDocumentContent();
      string fullName = // extract full name logic
      return fullName;
   }

    protected virtual string GetDocumentContent()
    {
        return CsvDocumentReader.GetContent(_documentPath);
    }
}

[Test]
public void ExtractFullName_DocumentHasFirstNameAndLastNameOnly()
{
   TestableCsvDataExtractor extractor = new TestableCsvDataExtractor("not important document path");
   extractor.ContentToReturn = "oren,ellenbogen";

   string result = extractor.ExtractFullName();

   Assert.AreEqual("ellenbogen", result);
}

public class TestableCsvDataExtractor : CsvDataExtractor
{
    public string ContentToReturn;
    
    public TestableCsvDataExtractor(string documentPath) : base(documentPath)
    {
    }
    
    protected virtual string GetDocumentContent()
    {
        return ContentToReturn;
    }
}

Pros: (1) if we can't refactor the original static class - it's a very fast & simple  solution to use (.vs. wrapping it).

Cons: (1) Not a perfect solution from an "elegant code" prospective.

 

 

 

I love to use option 3 if refactoring is too hard to (or not possible to) achieve but no doubt, option 1 will give you the best results if you're looking a few steps ahead.