Sponsor: Do you build complex software systems? See how NServiceBus makes it easier to design, build, and manage software systems that use message queues to achieve loose coupling. Get started for free.
It’s code review and design time. Recently, I recorded a video providing my thoughts on some code attempting to illustrate persistence ignorant for queries. I didn’t cover some specific API design decisions I disagreed with, around nullables and throwing exceptions. I’ll show samples of what I’d prefer the API to look like and why.
YouTube
Check out my YouTube channel, where I post all kinds of content accompanying my posts, including this video showing everything in this post.
Code Review
This post reviews code that was initially in this code review post, “Clean Architecture” and indirection. No thanks. I that post/video was mainly looking at the indirection being added around data access, but there were other design decisions that I wanted to address, so here they are.
Null
The first issue I have, not shockingly, is the intentional usage of null. Here’s the original code.
The thing to point out is on line 29, where FirstOrDefaultAsync() is being called. If there’s more than one record, we’ll get the first; if there are none, it will return null.
Now for its usage, this is a MediatR query handler, and take note of line 14, which checks if the orderResponse is null and then throws. To dig a bit deeper, let’s now look at the OrderReadService.
If we invoke this query from ASP.NET Core, we’d either need to catch this exception or have a global mapping for this custom OrderNotFoundException to an HTTP 4XX.
Expectations
What are the expectations around the data? FirstOrDefault was called. Do you mean the expectation is that the caller could pass an OrderId that either does not exist or possibly has more than one? And if there’s more than one, isn’t that hiding bad data? Or maybe we know there can only be one record, and we think that calling FirstOrDefault will be faster than SingleOrDefault()? Check out my video Single() or First()? Check out my video Understand the abstractions you use! as the results might not be what you think.
Now why would we need to use FirstOrDefault? Why would there be an exception of an OrderId being passed that wouldn’t exist? Well this depends on your context. If you’re working in an internal app, your inputs are often derived from your outputs elsewhere. Meaning if you’re generating a URI from the app, that’s going to be the input. An end user would have to go out of their way to provide bad input by changing the URI. If they did so, does it help them at all if you provided a HTTP 400 error saying the orderId didn’t exist, or if they got a HTTP 500?
If the answer is no, then being defensive in code isn’t saying you anything. If it does matter, hang on as I’ll show a different way instead of using null and throwing.
If we don’t need to be defensive, we remove nullable from our interface, can call SingleAsync() from the OrderReadService and now we can see how the query QueryHandler is simply calling the read service and returning its result. This better illustrates how useless the OrderReadService is and is just adding indirection.
We also don’t have to try/catch the OrderNotFoundException since we’re not expecting an invalid value. If an invalid value does occur, it will result in a HTTP 500.
Now you may have the use case where you want to handle the orderId being passed, which does not return a record from your database. In this case, you want to return a 400, for example. There are many valid reasons. This could be because you have a public API or are possibly deleting records in a multi-user environment. The problem with the first example is that exceptions are not explicit to the caller, and that might be what occurs.
Alternatively, you can define an option type to represent that the value exits (some) or does not (none).
Defensive
Do you need to be defensive? If you do, null and exceptions don’t need to be the answer. Exceptions aren’t explicit. As a caller, you have no idea without looking at the underlying code (and possibly levels of indirection) that an exception might be thrown. Nullable reference types help if you’re using a version of C# that supports it, but it doesn’t entirely solve the problem. You can avoid the usage of nulls and also be explicit by using something like an option type in these types of situations.
If you don’t need to be defensive, then don’t. Calling something like Single() when you expect a single record in the existing collection will expose underlying data issues. That’s a good thing. Be explicit.
Related Links
- “Clean Architecture” and indirection. No thanks.
- A promise is only as good as the code that makes it
Join CodeOpinon!
Developer-level members of my Patreon or YouTube channel get access to a private Discord server to chat with other developers about Software Architecture and Design and access to source code for any working demo application I post on my blog or YouTube. Check out my Patreon or YouTube Membership for more info.