Breaking dependencies to make your code testable

In my previous post I explained how you could use “Extract Interface” and “Subclass and override” as effective techniques for breaking dependencies and get existing code under test. In some situations there are other methods either more appropriate or easier to implement.

Simplify Parameters

When a method accepts a parameter that is difficult to construct in your unit testing framework, you an check the method for the actual information it uses from the object and replace the parameters with primitive parameters (or parameters that you can construct).

In the following example we have a class UploadManager which has a method that checks the file size of an uploaded file:

   1:  public class UploadManager
   2:  {
   3:      public bool CheckFile(HttpPostedFile File)
   4:      {
   5:          return File.InputStream.Length <= 1048576;
   6:      }
   7:   
   8:  }

This class would be really difficult to test because the class HttpPostedFile doesn’t have a public constructor. What you can do instead is accept a parameter of type stream and then write a test for the method:

   1:  public class UploadManager
   2:  {
   3:      public bool CheckFile(System.IO.Stream File)
   4:      {
   5:          return File.Length <= 1048576;
   6:      }
   7:  }

Now you can write your test class like this:

   1:  [TestClass()]
   2:  public class TestUploadManager
   3:  {
   4:      [TestMethod()]
   5:      public void TestCheckFile()
   6:      {
   7:          UploadManager target = new UploadManager();
   8:   
   9:          byte[] validFile = new byte[1001];
  10:          bool isValid = target.CheckFile(new System.IO.MemoryStream(validFile));
  11:          Assert.IsTrue(isValid);
  12:   
  13:          byte[] invalidFile = new byte[2000001];
  14:          isValid = target.CheckFile(new System.IO.MemoryStream(invalidFile));
  15:          Assert.IsFalse(isValid);
  16:      }
  17:  }

There’s one big disadvantage to this technique though. When you modify your method signature you have to change all the clients that are using your class. This can be really difficult. Also, if you don’t have all your projects that use this class in the same solution, you won’t know whether you’ve missed a place. As a work-around you could make an overloaded method to preserve the signature, but do all the work in your testable method:

   1:  public class UploadManager
   2:  {
   3:      public bool CheckFile(System.IO.Stream File)
   4:      {
   5:          return File.Length <= 1048576;
   6:      }
   7:   
   8:      public bool CheckFile(HttpPostedFile File)
   9:      {
  10:          return CheckFile(File.InputStream);
  11:      }
  12:  }

If you do this, there’s no need to actually change the client code. The old method is still in place and although it’s not testable, it doesn’t really matter, because it’s just forwarding the call to the method that can be tested. Ideally we would want everything under test, but sometimes you have to make a trade-off. This is one of those situations where the benefits outweigh the small disadvantage.

Breaking out classes

In my previous post I spoke about extracting interfaces in order to break dependencies. We can use that technique to break a dependency between two classes.
In the example however, we had two cleanly separated classes. If you have ever worked in legacy code, you know that this is usually not the case. Let’s look at a more realistic example:

   1:  public class OrderManager
   2:  {
   3:   
   4:      private SqlConnection _connection;
   5:      public OrderManager(SqlConnection Connection)
   6:      {
   7:          _connection = Connection;
   8:      }
   9:   
  10:      private void ExecuteSql(string Statement)
  11:      {
  12:          // execute the statement with the connection passed in the constructor
  13:      }
  14:   
  15:      public bool SaveOrder(Order Order, string Username)
  16:      {
  17:          bool isValid = false;
  18:          // check if order is valid
  19:          // ...
  20:          if (!isValid)
  21:              return false;
  22:   
  23:          // Build the SQL statement
  24:          // ...
  25:          string sql = "INSERT INTO ORDER ...<leave some opportunities for SQL injection here :) >";
  26:          try {
  27:              ExecuteSql(sql);
  28:          } catch (Exception ex) {
  29:              return false;
  30:          }
  31:          return true;
  32:      }
  33:  }

For the sake of brevity I have replaced large chunks of code with three dots. You can imagine hundreds of lines of code to appear there in a real legacy system.
Now, this method is impossible to unit test, because in our unit test you can’t have a connection to a database. You also can’t extract an Interface, because you don’t control the SqlConnection-class.

What we can do is extract this method to an external class. To do this, follow these steps:

  1. Create an empty class (let’s call it OrderProcessor in this case)
  2. Create a constructor that accepts a parameter with the type of the old class.
  3. Create a method with the same name
  4. Give it the same arguments as the original method.
  5. Copy the code to the newly created method
  6. If the method has a reference to any local variables of the old class, the compiler will tell you. Resolve these references, either by including them as parameters to the method or including them in the constructor of the new class. Sometimes this includes making some private methods visible to the new class (as in this example with the ExecuteStatement method).
  7. Change the method on the old class: instead of executing the code locally, create an instance of the new class and call the method.
  8. Use Extract Interface on the newly created class to break the dependency on the old class.

After step 7 you should have the following result:

   1:  public class OrderManager
   2:  {
   3:   
   4:      private SqlConnection _connection;
   5:      public OrderManager(SqlConnection Connection)
   6:      {
   7:          _connection = Connection;
   8:      }
   9:   
  10:      public void ExecuteSql(string Statement)
  11:      {
  12:          // execute the statement with the connection passed in the constructor
  13:      }
  14:   
  15:      public bool SaveOrder(Order Order, string Username)
  16:      {
  17:          OrderProcessor processor = new OrderProcessor(this);
  18:          return processor.SaveOrder(Order, Username);
  19:      }
  20:  }
  21:   
  22:  public class OrderProcessor
  23:  {
  24:      private OrderManager _mgr;
  25:      public OrderProcessor(OrderManager Mgr)
  26:      {
  27:          _mgr = Mgr;
  28:      }
  29:   
  30:      public bool SaveOrder(Order Order, string Username)
  31:      {
  32:          bool isValid = false;
  33:          // check if order is valid
  34:          // ...
  35:          if (!isValid)
  36:              return false;
  37:   
  38:          // Build the SQL statement
  39:          // ...
  40:          string sql = "INSERT INTO ORDER ...<leave some opportunities for SQL injection here :) >";
  41:          try {
  42:              _mgr.ExecuteSql(sql);
  43:          } catch (Exception ex) {
  44:              return false;
  45:          }
  46:          return true;
  47:      }
  48:  }

Our method  has now been moved to a new class. On line 42, you can see we’re calling back to the method on the old class to execute the SQL-statement. In order to do that, we had to make that method public. This is indeed not a good design, but as I mentioned before, sometimes these changes will improve the design, sometimes they will deteriorate your design. It’s all about trade-offs.

At this moment we still can’t create our unit tests, because now the new class has a dependency on the OrderManager and we still can’t construct that. However, this dependency is easier to break, because we control the OrderManager. We can extract an interface which will leave us with the following code:

   1:  public interface IOrderManager
   2:  {
   3:      void ExecuteSql(string Statement);
   4:  }
   5:   
   6:  public class OrderManager : IOrderManager
   7:  {
   8:   
   9:      private SqlConnection _connection;
  10:      public OrderManager(SqlConnection Connection)
  11:      {
  12:          _connection = Connection;
  13:      }
  14:   
  15:      public void ExecuteSql(string Statement)
  16:      {
  17:          // execute the statement with the connection passed in the constructor
  18:      }
  19:   
  20:      public bool SaveOrder(Order Order, string Username)
  21:      {
  22:          OrderProcessor processor = new OrderProcessor(this);
  23:          return processor.SaveOrder(Order, Username);
  24:      }
  25:  }
  26:   
  27:  public class OrderProcessor
  28:  {
  29:      private IOrderManager _mgr;
  30:      public OrderProcessor(IOrderManager Mgr)
  31:      {
  32:          _mgr = Mgr;
  33:      }
  34:   
  35:      public bool SaveOrder(Order Order, string Username)
  36:      {
  37:          bool isValid = false;
  38:          // check if order is valid
  39:          // ...
  40:          if (!isValid)
  41:              return false;
  42:   
  43:          // Build the SQL statement
  44:          // ...
  45:          string sql = "INSERT INTO ORDER ...<leave some opportunities for SQL injection here :) >";
  46:          try {
  47:              _mgr.ExecuteSql(sql);
  48:          } catch (Exception ex) {
  49:              return false;
  50:          }
  51:          return true;
  52:      }
  53:  }

Now the OrderProcessor has no dependencies anymore and we can create a fake to write our unit test for the SaveOrder-method.

This is not a simple refactoring and you should only revert to this if you have no other option. It’s a lot riskier than the other methods explained so far, but sometimes it can help you to break a very though dependency such as the one just explained.

Usually it also brings in other disadvantages such as breaking encapsulation on certain classes. You need to keep these things in consideration when you apply this technique.

Comments are closed.