Unused Constructor Dependencies

Some classes simply require multiple dependencies that don’t always get used.  When testing such a class, there are several options for supplying the constructor parameters that will not be used.  I prefer to write tests that are as intention-revealing as possible. 

To demonstrate, here is a simple example of a payment processing class.  Think of the credit card machines that are found at the local grocery store.  The basic flow is for the customer to approve the charge amount, then swipe their credit card or debit card and complete the transaction.  Below, the PaymentProcessor is responsible for displaying the current charge amount to a user, verifying that they accept the amount, and then sending transaction information to the bank to request funds.  Here is an example implementation:

 

   1: public class PaymentProcessor

   2: {

   3:   private readonly IBankInformationReader _bankInformationReader;

   4:   private readonly IBankService _bankService;

   5:   private readonly IUserVerification _ui;

   6:  

   7:   public PaymentProcessor(

   8:     IBankInformationReader bankInformationReader, 

   9:     IBankService bankService, 

  10:     IUserVerification ui)

  11:   {

  12:     _bankInformationReader = bankInformationReader;

  13:     _bankService = bankService;

  14:     _ui = ui;

  15:   }

  16:  

  17:   public bool ProcessPayment(double amount)

  18:   {

  19:     if (!_ui.VerifyAmount(amount))

  20:       return false;

  21:  

  22:     var bankInfo = _bankInformationReader.GetBankInformation();

  23:  

  24:     return _bankService.RequestFunds(amount, bankInfo);

  25:   }

  26: }

 

The PaymentProcessor depends on IBankInformationReader, IBankService, and IUserVerification.  If the user declines the payment amount, then processing should stop immediately.  You can see this behavior above in lines 19-20.

Under this scenario, then PaymentProcessor should never interact with either the IBankInformationReader (used for reading the credit card swipe) or the IBankService (used for submitting the transaction).  My preference is to pass null for the dependencies that shouldn’t be used, but to do so in an intention-revealing way.  Here is an example:

 

   1: [Test]

   2: public void should_not_charge_if_the_user_declines_the_amount()

   3: {

   4:   double amount = 4.50;

   5:  

   6:   IBankInformationReader cardReaderShouldNotBeUsed = null;

   7:   IBankService bankServiceShouldNotBeUsed = null;

   8:   

   9:   var ui = Stub<IUserVerification>();

  10:   ui.Stub(x => x.VerifyAmount(amount)).Return(false);

  11:  

  12:   var processor = new PaymentProcessor(

  13:     cardReaderShouldNotBeUsed, bankServiceShouldNotBeUsed, ui);

  14:  

  15:   processor.ProcessPayment(amount).ShouldBeFalse();

  16: }

 

Rather than simply passing the value null to the constructor in lines 12-13, I’ve used some local variables.  This allows the test to clearly show intent.  In this case, the test states that PaymentProcessor should never use the bank service or the card reader when the user declines the transaction.  And, by using a null, we know that the test will fail if the PaymentProcessor tries to use those dependencies. 

There is another option which is equally intention revealing but somewhat more noisy.  We could use a mocking framework to generate the dependencies and then verify at the end of the test that they have never been used:

 

   1: [Test]

   2: public void should_not_charge_if_the_user_declines_the_amount()

   3: {

   4:   double amount = 4.50;

   5:  

   6:   var cardReader = Stub<IBankInformationReader>();

   7:   var bankService = Stub<IBankService>();

   8:   

   9:   var ui = Stub<IUserVerification>();

  10:   ui.Stub(x => x.VerifyAmount(amount)).Return(false);

  11:  

  12:   var processor = new PaymentProcessor(

  13:     cardReader, bankService, ui);

  14:  

  15:   processor.ProcessPayment(amount).ShouldBeFalse();

  16:   cardReader.AssertWasNotCalled(x => x.GetBankInformation());

  17:   bankService.AssertWasNotCalled(x => x.RequestFunds(0, null), 

  18:     o => o.IgnoreArguments());

  19: }

The problem that I have with this test is all the noise on lines 16-17.  I would rather reveal this intention through some well-named variables.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Eric Anderson

Eric has 6 years experience in software development with the last 3 being in Agile shops. He is now spreading the Agile love as a Senior Consultant with Headspring Systems in Austin, TX. Eric loves to share his passion for software development, especially Test-Driven Design, with anyone that will listen. Eric is also a Hudson Continuous Integration fanboy. He recently published “Hudson Continuous Integration Server” in the May/Jun ‘09 issue of C.O.D.E. magazine to help .Net’ers get started with Hudson. Outside of software, Eric’s passions include his family (wife, son, 2 daughters), fixing things around the house, and volunteering with various local churches and organizations.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

12 Responses to Unused Constructor Dependencies

  1. Travis says:

    I’m assuming creating a second constructor that only contains dependencies that will be used for ProcessPayment() is a bad pattern?

  2. Paco says:

    Why don’t you use automocker for this?

  3. Eric Smith says:

    That first test of yours will throw a NullReferenceException if there’s an attempt to GetBankInformation or RequestFunds when either of the relevant services aren’t defined. Is this intention revealing? Dunno, just doesn’t sit well with me.

  4. If I’m doing constructor injection with a public constructor, I tend to check the arguments vs. null and throw ArgumentNullException.

  5. Dokie says:

    I don’t see how this does anything to help test the method since in the real positive case the injected interfaces are not expected to be null. In this test the only way it would attempt to use the null interfaces is if the stubbed return was altered. I would personally Guard against null injected values in the c’tor.

  6. Dan says:

    I agree with you that the second test is less desirable.

    The reason I see the test as less than desirable is because if the test doesn’t care about the RequestFunds() method why should we document it in the test? This could easily cause the test to become brittle. Why make a test depend on something that has nothing to do with the success or failure of the the unit in question?

    For example, if a parameter was added to the RequestFunds() method then this test would not compile and we would have to fix it before continuing.

    In an isolated case it doesn’t look like a big deal but on a larger scale, with a few thousand tests, you could see how the cost of maintaining the tests go up and the original intent of the test becomes less clear.

    Good post!

  7. While I dig the idea I don’t think it is wise to use it in a wild because it makes your tests fragile (now they are testing more then one thing and have more then one reason to fail)

    IMHO. If this null behavior is not so important for you you should remove it from test and if it is you should create a new test for that null behavior and appropriate name.

    Nikola Malovic

  8. Ugh. I just reread your post much slower this time. The below is what I was originally going to say. Now I’ll just say this: I would never verify that a dependency is NOT used by a class in a test. This is clearly a shabby design; yes it happens sometimes, but when it does, I immediately refactor and figure out what’s wrong with my design.

    While it is certainly possible to have unused dependencies in your ctor, it is unwise to code your tests as if you know that, or any of your code, for that matter. That implies that your class is doing too many things at once, and that it is not very cohesive. It is also an indication that you are testing against the private parts of your system, which is incredibly vulnerable and brittle.

    Another point that I’ve realized lately (not necessarily all that related to the post, but definitely to some of the comments) is that null should never be passed into the ctor of an object. This implies that the outside world knows when a certain dependency doesn’t need to be there, and that defeats the purpose of encapsulation.

  9. Charris says:

    @Kyle, good points, I totally agree. I’ll likewise make checks against the null ref, I don’t want others thinking there are hidden behaviors for a class.

  10. I agree that the design is not optimal. However, there are some types of classes where this is totally unavoidable. For instance, web controller classes. I may have different actions on a page that use different dependencies. In that case, I’d rather be intention revealing through well-named variables rather than simply passing null.

    Any method that uses multiple return points is another example. Sometimes leaving those multiple return points together is better for the overall design.

    I agree that the ProcessPayment() method as shown above is not optimal. I would probably refactor out some of the responsibilities myself. But, there are simply some classes that cannot be refactored.

    As to the NullReferenceException that will come from erroneously using the null dependencies, I would rather have that than a more obscure failure. For example, a default stub might return null or swallow a call to a void method. This may or may not cause an error in the test. In my experience, when these tests do fail because of the default behavior of the stub, those errors are more difficult to debug than a NullReferenceException with a stack trace that points directly to the problem.

    At the end of the day, much of this is simply preference. I’m not strictly dogmatic about using named nulls when I have to, I simply like that option better than unnamed nulls or automocking containers.

  11. Scott_M says:

    It would be really cool if IOC containers could be smart enough to deal with the scenario of multiple dependency injection constructors. This would eliminate the scenario of having sometimes unused dependencies. I guess one could argue that if you find your self in the situation of having unused constructor dependencies, you should consider splitting your class into multiple classes.

  12. David Miller says:

    The other approach which i’ve seen is to use the “strict” behaviour of mocks. If any method is called that is not explicitly setup then an exception will be thrown.

    I prefer to have a helper method:

    public static TService EnsureNeverCalled()
    {
    var mock = new Mock
    (MockBehavior.Strict);
    return mock.Object;
    }

    I also like to add a number of Guard clauses to the constructors of my objects – to ensure that no null dependencies are passed through. These guard clauses only verify method arguments as not null, not empty and (to a lesser degree) specific conditions. See http://ajdotnet.wordpress.com/2009/08/01/posting-guards-guard-classes-explained/

Leave a Reply

Your email address will not be published. Required fields are marked *

*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>