A number of times over the years I have seen code which wants to be defensive (which is good) and check inputs to methods however isn’t using the power of the framework to help.
A number of basic building blocks, such as Guid and int, have such methods which can help - they are the TryParse
methods.
The Problem
I’d imagine we’ve all seen code similar to this over the years.
public void MethodName(string input)
{
if (string.IsNullOrWhiteSpace(input))
{
throw new ArgumentNullException();
}
Guid parsed;
try
{
parsed = Guid.Parse(input);
}
catch (FormatException e)
{
throw new ArgumentNullException();
}
if (parsed == Guid.Empty)
{
throw new ArgumentNullException();
}
// do the actual work here
}
Let me start off by saying the above isn’t completely bad. Checking at various points works great. The one part which is “demoware” is where the FormatException
is caught and an ArgumentNullException
is thrown. This can be replaced by a more appropriate exception however a FormatException
can imply an ArgumentNullException
so it’s not all bad.
How can this be improved?
Unit test
First we need a unit test to prove that everything stays the same as we refactor. As you can see from the unit test this is why FormatException
was caught and an ArgumentNullException
was thrown.
[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData("1234589")]
[InlineData("qwertyuiop")]
[InlineData("00000000-0000-0000-0000-000000000000")]
public void Test(string input)
{
Action result = () => MethodName(input);
result.Should().Throw<ArgumentNullException>();
}
Using xUnit and the Theory/InlineData attribute combinations you can test multiple scenarios with only 1 test body.
Potential Solution
public void MethodName(string input)
{
if (string.IsNullOrWhiteSpace(input))
{
throw new ArgumentNullException();
}
if (!Guid.TryParse(input, out var parsed))
{
throw new ArgumentNullException();
}
if (parsed == Guid.Empty)
{
throw new ArgumentNullException();
}
// do the actual work here
}
With Guid.TryParse
and some modern inline out parameter syntax it looks a lot better; cleaner, easier to read and simpler to maintain.
The issue with above is you are checking the input
variable multiple times. We’re checking before we use TryParse
and then inside the inner workings of TryParse
it is checking again.
So let’s clean it up a bit more.
public void MethodName(string input)
{
if (!Guid.TryParse(input, out var parsed) || parsed == Guid.Empty)
{
throw new ArgumentNullException();
}
// do the actual work here
}
Conclusion
As you can see it’s easy to work with the framework tools to make code easier to read and maintain. This also allows us to concentrate on writing functionality and adding value instead of duplicating framework code.
The above is an example of how code can be built up and unit tested during development and lends itself well to being refactored before shipping.
Remember; Red, Green, Refactor!
Any questions/comments then please contact me on Twitter @WestDiscGolf