CODEWITHSUNDEEP
c#.netunit-testingtdd
Jon
Jon

Reputation: 40032

TDD & Design Advice

I have a method which calls an Action<string>

public Action<string> DisplayError;

public void MyMethod()
{
  DisplayError("ERROR");
}

In this method I want to call DisplayError however I can see that if DisplayError is null it will thrown an exception.

I can run a test that proves that it will throw an exception.

So I know I want to add a if (DisplayError != null) to my code but I feel this design is wrong somehow. Maybe the test should be different?

Upvotes: 0

Views: 113

Answers (3)

Lunivore
Lunivore

Reputation: 17602

Rather than making Action a public field, make it a property or pass it into the method. You can then do the null checking at that point. I also doubt that you need to retrieve it after setting it, so either:

_displayError = (s) => { throw new ArgumentNullException("Please tell me what to do with this exception!"); };

public Action<string> DisplayError
{
    set 
    {
        if (value == null) { throw new ArgumentNullException("I need this!"); }
        _displayError = value;
    }
}

public void MyMethod()
{
    _displayError("ERROR");
}

Or:

public void MyMethod(Action<string> displayError)
{
    if (displayError == null) { throw new ArgumentNullException("I need this!"); }
    displayError("ERROR");
}

The first of these shouldn't require any code changes from your consumers.

Of course, if you're just working with a team who are using your code, you shouldn't need to null-check at all. Just find out who's using your code wrong and have a friendly chat to work out where the misunderstanding is. Defensive programming is not a good substitute for a team whose members trust each other, and null-checking is not as effective as having a clearly defined, easy-to-use interface.

You'll need 2 tests:

My class:

  • should allow the consumer to handle any error
    • Given I'm going to produce an error for whatever reason (set context here)
    • When I produce an error (call the method, let the error handler set some variable)
    • Then I should give the error to the handler (check the variable that got set)
  • should make sure an error handler is attached
    • Given I might produce an error (you can code what you like here because it's irrelevant)
    • When the consumer calls me without an error handler (call the method with a null error handler)
    • Then I should immediately ask the consumer not to do that (check for an exception)

Upvotes: 1

Austin Salonen
Austin Salonen

Reputation: 50215

It depends entirely on context.

Is the user of this class required to set the value based on a contract? Then it should throw an exception.

Is it optional? Then your check is valid but if you just default DisplayError to an empty Action, you'll get similar behavior 

DisplayError = s => {};

Your approach also leads to the possibility that the user can set DisplayError to null and in that case, only you can decide what's valid. If you set that to a property instead of a field, you'll give yourself more options.

_displayError = s => {};

public Action<string> DisplayError
{
   get { return _displayError; }
   set 
   { 
       _displayError = value ?? (s => {}); 
       /* or throw on null */
   }
}

Upvotes: 1

dwerner
dwerner

Reputation: 6602

I believe it was in a talk from Steve Yeggie that I heard the point: something to the tune of 'while statically typed languages do protect you from a wide class of errors at compile-time, they do not negate the need for null-checking.'

I find this perfectly normal.

I'm just drinking my morning coffee, so let me know if I completely missed the mark.

Upvotes: 0

Related Questions