The ThrowIf* Method Pattern

The ThrowIf* Method Pattern
   There is a simple pattern that is extremely effective and increases source code readability greatly. Sadly this pattern is underused and many developers do not even think about it. I am talking of course the ThrowIf methods used to throw exceptions when validating state and arguments.

   The pattern is simple – write specific methods to check conditions and throw exceptions and name them with ThrowIfCondition. For example if we are writing List<T> and want to implement the RemoveAt(int index) method we might validate the argument like this.

public void RemoveAt(int index)
{
   if(index < 0 || index >= Count)
   {
       throw new ArgumentOutOfRangeException(nameof(index));
   }
   //actual logic…
}

We can extract that in a method that makes it slightly easier to read because code that resembles English sentences is easier to read

public void RemoveAt(int index)
{
   ThrowIfIndexInvalid(index);
   //actual logic
}

private void ThrowIfIndexInvalid(int index)
{
   if(index < 0 || index >= Count)
   {
       throw new ArgumentOutOfRangeException(nameof(index));
   }
}

This is a small improvement but it piles up when the same validation logic is used in multiple places. For example in the List<T> case the validation logic can be reused in places where an index is passed as an argument like Insert and the getter and setter for the indexer.

   One place where the pattern shines particularly brightly is ASP.NET Web API. Web API has the concept of HttpResponseException. When thrown this exception is caught by Web API and converted in the corresponding HTTP response. One can argue that this practice is using exceptions for flow control but a good counter-argument is that non-success status codes in HTTP are effectively exceptions so it is OK to represent them as exceptions in our backend code. In any case I have found that they work very well and the code becomes very readable when we combine these exceptions with the ThrowIf pattern. For example when you accept some data you check if it is valid like this

public IHttpActionResult Post(Comment comment)
{
   if(!ModelState.IsValid)
   {
       return BadRequest();
   }

   //save the comment in the database or something
}

This is the normal flow that returns a result object and BadRequest is a helper method inherited from ApiController that returns a bad request result. But what if you want to return a normal object for example the comment with the populated ID. Then we can change the method like this:

public Comment Post(Comment comment)
{
   if(!ModelState.IsValid)
   {
       throw new HttpResponseException(new HttpResponseMessage(HttpStatusCode.BadRequest));
   }

   //save the comment in the database and return it
}

And then we might want to put the validation errors in the response and this becomes a lot of code to repeat in half of your actions. So here are a couple of methods that I put in my base controller and use in my actions

protected void ThrowBadRequestIfModelStateInvalid()
{
   if (!ModelState.IsValid)
   {
       var messageBuilder = new StringBuilder();

       foreach (var modelItem in ModelState)
       {
           foreach (ModelError error in modelItem.Value.Errors)
           {
               messageBuilder.AppendLine(error.ErrorMessage);
           }
       }

       ThrowBadRequest(messageBuilder.ToString());
   }
}

protected void ThrowBadRequest(string message)
{
   var responseMessage = new HttpResponseMessage(HttpStatusCode.BadRequest)
   {
       Content = new StringContent(message)
   };

   throw new HttpResponseException(responseMessage);
}

So here is what the comment creation method may end up looking like after applying the pattern a twice.

public async Task<Comment> Post(CommentCreateDto commentCreateDto)
{
   ThrowBadRequestIfModelStateInvalid();
   Post post = await PostService.GetAsync(commentCreateDto.PostId);
   ThrowNotFoundIfNull(post);

   //create the comment in the database

   return comment;
}

First we check if the data is valid (max length and things like that), then we check if the comment is attached to a valid post and throw 404 if the post cannot be found. Then we create the comment and return it back to the caller. Note that this code lacks actual if statements. All the branches are encapsulated in the validation methods. We are not repeating the checks and thus avoiding code duplication.

   While currently I am not aware of any downsides of this pattern in the future using it to check for nulls might be problematic. If C# introduces nullability tracking it will probably not be aware that the ThrowNotFoundIfNull method checked for null and will therefore force a null check again. One solution would be to implement the method like this

protected T ThrowNotFoundIfNull<T>(T? o)
{
   return o ?? throw new HttpResposneException(…);
}

and then use it like this

post = ThrowNotFoundIfNull(post); //the compiler is now aware that the value is non-nullable

I am not sure if this particular generic implementation will be possible with what C# will implement but I hope there will be a solution to keep using the pattern in this downgraded way. Ideally the compiler will be aware of the check even without reassignment. I think this is possible to do with Code Contracts and postconditions but I think the proposal for C#’s nullability tracking is much less powerful than what Code Contracts can do. In any case the pattern will still be useful just like today in all other situations.
Tags:   english programming 
Posted by:   Stilgar
22:59 13.08.2017

Comments:

First Previous 1 Next Last 

Posted by   Nikolay (Unregistered)   on   21:19 14.08.2017

I would prefer to call these methods ValidateWhatever as this is what they do. Always prefer to name the methods after what they do, now how they do it.

Posted by   TheUnrepentantGeek (Unregistered)   on   22:21 20.08.2017

The biggest drawback I've seen when using this approach occurs if/when someone takes a shortcut with exception handling - perhaps as a part of logging - and there's a bug where the exception is thrown incorrectly.

If the call stack is lost, all we know is which ThrowIf() method triggered the exception. This is fine if the method is only used in one place - but if reused, that lost information can make it much harder to diagnose the problem.

Of course, the fix isn't to avoid this pattern - but to fix the logging/exception handling so the call stack isn't lost.

Posted by   David (Unregistered)   on   20:46 21.08.2017

My preferred naming convention is `Guard<InvalidConditions>`, which I think does a good job of not explaining how something is done, as @Nikolay suggests, but doesn't come off as too vague.

For example:

`GuardAgainstNull<T>(T? t)`

Seems to do good job of broadcasting that this method will take some preventative action if null.

Posted by   Stilgar   on   18:48 23.08.2017

Guard is probably somewhat better in vacuum but ThrowIf* is somewhat established already including in framework code.

First Previous 1 Next Last 


Post as:



Post a comment: