Refactoring legacy code for unit testing

I'm trying to refactor some code for unit testing and was hoping you could critique it.

This is the original method:

public class MyNonRefactoredClass
{
   public List DoSomething()
  {
      List data = GetData();

      if (data.Count == 0)
        return new List();

      data = GetFormattedData(data);

      if (data.Count == 0)
        return new List();

      //Do some more stuff

      return data;
   }

  public List GetData()
  {
      //Do db query and return data
  }

  public List GetFormattedData(List Data)
  {
      //Do some logic on Data variable. Might return empty if data is not what I like
  }

}

Here is my new approach. I have done it like this as I believe I need an entry point to try and access methods that I want to test.

public class MyRefactoredClass
{  
  public List DoSomething()
  {
    //Dependency Injection needs to work somehow here
    MySeparateClass newClass = new MySeperateClass(new DataProvider());
    List dbData = newClass.GetData();
    if (db.Data.Count == 0)
      return new List();
    dbData = newClass.GetFormattedData();
    if (db.Data.Count == 0)
      return new List();
    //Continue method doing other things...
    }
}

public class MySeparateClass
{
  private IDataProvider dbProvider;

  public MySeparateClass(IDataProvider DataProvider)
  {
    dbProvider = DataProvider;
  }

  public List GetData()
  {
    //Do DB stuff
    return dbProvider.GetData();
  }

  public List GetFormattedData(List Data)
  {
    //Format
  }
}

[Test]
public DoSomething_DBReturnsNoData_ReturnEmptyList()
{
  //Setup mock of IDataProvider
  //Tell mock to not return data from GetData
  //Assert that result.count == 0
}

[Test]
public DoSomething_FormattedReturnsNoData_ReturnsEmptyList()
{
  //Setup mock of IDataProvider
  //Tell mock to return some data from GetData
  //Setup mock of MySeparateClass
  //Tell mock to return no data from GetFormattedData
  //Assert that result.Count == 0
}

Looking at your code I see three responsibilities:

• the Data Provider (component that hits the db)

• the Data Formatter

• the business service (your class) that uses the other two components to execute a business flow.

According to the SRP, one class should have one responsibility, so we should end up having three classes.

Therefore, I'd separate out the data provider and formatter interfaces, like so:

public interface IDataProvider
{
    List GetData();
}

public interface IDataFormatter
{
    List GetFormattedData(List data);
}

and refactor our business class to use these dependencies:

public class RefactoredClass
{
    private readonly IDataProvider _dataProvider;
    private readonly IDataFormatter _dataFormatter;

    public RefactoredClass(IDataProvider dataProvider, IDataFormatter dataFormatter)
    {
        _dataProvider = dataProvider;
        _dataFormatter = dataFormatter;
    }

    public List DoSomething()
    {
        var data = _dataProvider.GetData();

        if (data.Count == 0)
            return new List();

        data = _dataFormatter.GetFormattedData(data);

        if (data.Count == 0)
            return new List();

        //Do some more stuff

        return data;
    }
}

Now you can write all relevant unit tests, for each system component in isolation:

• you can test RefactoredClass providing your mocks/stubs for the two dependencies

• you can test the DataFormatter(s) in separate unit test(s)

• you can test the concrete DataProvider using integration tests (since this component touches the database)

Regarding Depenedency Injection, this is how you should compose this component somewhere in your application root:

var dataProvider = new SqlDataProvider();
var dataFormatter = new MySlickDataFormatter();

var businessComponent = new RefactoredClass(
    dataProvider,
    dataFormatter);

This technique of manually instantiating the needed component and its dependencies is called "Poor man's DI". If things get any more complicated, you should consider using an IoC container for this.

Update:

Your tests make sense, but there are more tests to be added. For example, the formatting logic is not tested.

Also, regarding your second test - you have no mocks in there. The MySeparateClass instance is the system under test and the IDataProvider instance is a stub. Your test does make sense; that's just a small naming issue.

A mock is basically a fake instance that you assert against.

If you google for "mocks and stubs" you'll find plenty of articles on this subject that you can read. I also recommend the book "The Art of Unit Testing" by Roy Osherove - a great resource on this subject.

StackExchange Code Review Q#8420, answer score: 6