One of the problems with static members or static classes is that you can't mock them for proper unit-testing. Instead of taking this fact for granted, let's demonstrate it. Assume that we have the following classes (please note, this is just an example, not production code or anything that I'll be proud of later on):
public class CsvDataExtractor
{
private string _documentPath;
public CsvDataExtractor(string documentPath)
{
_documentPath = document;
}
public string ExtractFullName()
{
string content = CsvDocumentReader.GetContent(_documentPath);
string fullName = // extract full name logic
return fullName;
}
}
[Test]
public void ExtractFullName_DocumentHasFirstNameAndLastNameOnly()
{
CsvDataExtractor extractor = new CsvDataExtractor("c:\test.csv");
string result = extractor.ExtractFullName();
Assert.AreEqual("ellenbogen", result);
}
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.
Here are our options to solve this dependency so we could test ExtractFullName method by itself:
1). Refactor the static class into "instance" class and implement some sort of IDocumentReader interface.
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.
2). Wrap the static class with an instance class and delegate calls:
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.
3). Use "protected virtual" method to call the static member and override it:
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.