Code Review & Refactoring to a better design 

Option is a monad which makes it composable. However, for some reason almost all uses in C# I see in the wild - like here - is a glorified way to represent a missing return value. With nullable reference types there's good support for that in the C# type system - just annotate your type to indicate that it can be null. Until you start using the composability of Option it doesn't really add any value in my opinion.

(specific to SQL Server) If you're dealing with a Unique Index or Primary Key, there is 0 difference between `TOP(1)` (First/FirstOrDefault) and `TOP(2)` (Single/SingleOrDefault) as once the record is found it is returned. When you're NOT using a Unique/Primary Key for the index to know that there is 1 results, you can have 2 different behaviors. 1) If you use (First) TOP(1) without an order by, it will stop execution on the first result 2) if you use (First) TOP(1) with an order by, it will cause the whole table to be scanned as the sort is evaluated after all results are found, then TOP(1) will be applied. Single in the example from Derek is 100% totally fine and there will not be any performance difference. Single is handy for data integrity if you expect 1, but you really should be keeping that integrity in the database with a unique index and not relying on your application code to ensure there is only 1 result.

I agree with much of what you said here. But even if this is a private app or a single app usage API, an order could be on a grid of one user that another user has since deleted. If the API returns a 500, what does the client do? Does it retry? Does it give up? Does it assume the order isn't there? In addition, logging a 500 response our logging aggregator and monitoring would consider that an ERROR and might alert if too many 500s are returned in x amount of seconds. I'd also much prefer that 404 so the client can respond accordingly and give the user a real message like "That Order Doesn't Exist" or whatever. So, yea, I know it wasn't the full point of your video, but I think returning a 500 when 404 is the real issue is a bad choice.

It's such a relief to hear these opinions that i completely agree with.

We used something similar to Option at work, along with simple result for why something wasn't returned. With more complex API it really started to clutter handlers and other parts of code with all the Option checks if something was actually returned or not. That was the case especially if we had to perform more complex operation than simple GetById. Basically it came down to literally same thing in the end - check if something is null/option returned something. We ended up implementing global exception handling (that we transform to proper http status code) and always expect data to be returned. It might be a hit on performance, but it improved clarity of code for us and made it more manageable, far less if's and checks were required.

It seems like almost everyone can see why Goto Label leads to confusing control flow. Yet, they do not seem to understand that exceptions are basically Goto's but on the call stack. What you end up depends on what error you throw, and the outer context of who is catching it. They truly are meant for situations where you cannot resume your app in a valid state without unwinding the call stack to a location where the app state is valid.

I'm curious, does Option really improve anything over nullable? I feel like nullability is preferred past C#8 since there's amazing support for it now. The issue I usually face with Option returning methods is that it ends up with a lot of functional composition which clutters the code a lot. I guess it's probably a matter of style and code conventions. I'm with you on the exception part though. Exceptions should not be used for control flow, but I still consider Exceptions as a very important piece of error handling since it's the only true way of producing unrecoverable errors which the client can't accidently ignore if the return type is discarded.

two comments :) I see that conveniently the method in the controller had a try catch block, that wouldn't be the case if you are using business exceptions with global exception management. The advantage of that approach compared to the approach reviewed here, it's that you can consolidate error handling into a single location instead of scattering it across different parts of your codebase as needed with Option. I don't want to say one approach is better than the other, but don't buy either that global exception management it is a bad thing, it is well known and documented in the bibliography as a good practice. It is a matter of taste, good developer would get the benefits of both. The other thing is about Single vs First, depends on the context but most of the cases when searching by ID, the ID is unique, and most of the DB engines FirstOrDefault is going to be faster than SingleOrDefault.

In a previous project, and our current project, we decided to separate the definition of errors that the user can potentially fix and errors that the user can't fix. Errors of the first category can be returned as a 400 Error Result object. These are input validation errors and business (validation) errors. Our Handler returns a Result, the API converts the Result. We don't throw exceptions in this case. Errors of the second category are "thrown" to the global exception handling middleware to deal with. These usually result in some form of 500 error (or equivalent) and a detailed error log. These are mainly "the database is down", "an external service is down", "we ran out of memory..." types of errors. NotFound errors / exceptions are bit of both. If we're expecting that it's possible that a certain concept doesn't exist in the DB anymore then we could return an Error, otherwise we throw an Exception which results in a 404 with a detailed message.

why use Option type when the compiler can warn you when you are accessing a member of a nullable value?

Generally agree with public vs internal differences, but for such a small amount over upfront overhead, I think it's worth just thinking about all calls as being 'public' and handling them as potentially problematic.

Good example of how/why to use Option over exceptions. I'm not totally sold on the .Single over .First though. There are many cases when there will be a significant performance hit for using .Single over .First. That said, it is (or should be) obvious that the order id is unique (probably -- but not always, some system may wrap the order id around after 4-6 digits), but if it wasn't, then using .First without an .OrderBy would be just as bad if not worse. But, if you already know that level of detail about the implementation, then you should already know that there will only ever be at most one record, and then the only difference between .First and .Single is a possible performance difference on non-unique columns. For the above reasons, during a code review, I would flag .Single to be changed to .First unless there is a clear reason otherwise, but if I was on the other end, I would not contest a request to use .Single in my own code. Subjective answer.

Can you please talk about the option type?

One challenging thing is keeping "domain" knowledge or requirements within the domain project. I think the "not found" might be a domain requirement so it should be handled within the handler. However to accomplish that the GetOrderByIdAsync could return a nullable OrderResponse and the handler could construct the option, but I think using option at the repository is also valid. What are your thoughts? Is the not found domain logic in your refactor still in the domain? My conclusion is that it's still in the domain simply because the handler chose which repository method to call, it just so happens the code of the handler is very simple.

Hope you do loads more code review videos! I could definitely do with fixing some bad habits

Even if it’s a private api you might have a multi-user race condition scenario where one user requests a resource that a second user just deleted. There might be other ways to solve that, but that might be a reason to still check if the resource exists. However checking if there are more than one resource with that id should not be necessary because then you have allowed your system to be put in an invalid state so your problem is where you update your state.

Not related to this specific video and occurance, but want to add to the First vs Single topic and correct me if I am wrong! Lets suppose the item we look for does indeed exist! First(OrDefault) will go through a collection until the item is found and be done with it. Single(OrDefault) will always go through the entire collection. Now if you work with very big collections and have many calls that go through it, this might matter in performance. If you know that on the DB side the uniqueness is ensured anyway you might be ok with using First just to be a bit faster. Thoughts?

When you are searching for a single entity with the primary key you should use Find. There will never be a duplicate record as the id is unique and will throw an error on insert, therefore First Vs Single does not mask or unmask a duplicate problem. Correct me if I am wrong 😊

I thought FirstOrDefault() was used here because of performance optimization, so it doesn't have to go through all data to ensure only one result exists, because the signle result is ensured by quering by primary key. Did i misunderstood it?

Also, I feel like there should be a wrapper around the Results Object to indicate the number of records that were found. In this way we can communicate if anything was found and keep nulls out of a system as they do nobody any good!

Great Video! @ 3:42 What is your view of removing the curly braces by using an arrow (=>) since there is only one line of code inside the curly braces? Generally speaking, when would you use an arrow function? Love to hear your thoughts.

I am torn on this. I value the explicit nature of the return types, however I also value not having too many lines and complex structures in my code. In a Spring Boot Application you can have a global exception handler and thus I like to throw my (custom) exceptions, catch them globally and then handle the individual cases there. That keeps all my code clean (i.e. small and readable) but I live with the knowledge that there is a side effect dealing with all misses... On a side note - 500 gives me itches. I live by the philosophy that I - the developer - screwed up if there is any 500 coming from my app (ignoring third paries e.g. database connection lost). And I don't like to screw up.

Would love to see a video of you refactoring this further to remove the indirection created by the IOrderReadService

When working with data that can be concurrently changed, someone could have deleted a record you are trying to open(the view is stale) and then you can use 404 to actually tell in the client side that the record wasn't found anymore.

Warn me if I am wrong but, Single method will make database search until it founds the 2nd one to throw exception or it will go to the end of the records to ensure there is only 1. But First method will stop immediately when it finds one. If you are searching a primary key, you dont have to search your entire database. Use First() if you 100% know there is only one record. However, I am using Single method too, in appropriate places.

For all new folks here, try to avoid nulls for something more meaningful like Option or other monads like Maybe, avoid throwing exceptions for cases that you expect to have. Good video.

SingleOrDefault() is a performance impact when using in-memory collections. If you're using an underlying database provider, you won't be fetching more data that meets the criteria of the query. If only 1 record matches the Where(), that's all that's returned from the database. When you have an in-memory collection, Single() is a performance impact because it needs to confirm there is only matching record. However when you're using a database provider, the data returned won't be over fetching as it will only return that matches, regardless of the limit defined. eg, SELECT TOP 2 * FROM Table WHERE X=Y. If there's only one record that matches, you'll only get one record.

Very good thoughts. I've wrestled with the idea of throwing specific exceptions a times, mainly because I have heard "you're supposed to." But in practice, I never found it extremely helpful to always inspect the implementation of the consumed class in order to know what action to take after an exception is thrown. Not that you should never catch specific exceptions and handle them, but rather given the appropriate context, relying upon explicit return types does seem like a better approach.

Derek, I've been using FluentResults for the past few years, so another take on the Options pattern, but this leads to some methods overflowing with IsFailed checks. Any idea how to handle this? Match() firing the next step on success? Or maybe something better?

hey dumb question; since I am a book freak. The bottom book is definetely DDD by Eric Evans, but what's on top??

This is an approach I've commonly seen and applied with spring boot

Just want to say it’s hard to follow your clicks, maybe add an extension so we can direct our attention to the correct spot or increase the size of your pointer cursor

Let's not forget more than one person can load the page and one deletes it before the other updates it. I also advocate not using a 404 or 500 explicitly, IMO they should be left to the web server to handle or they become ambiguous. Agree with everything else though, throwing and try/catching is well misunderstood and more often than not used for control flow and not exceptional flows.

Very good video, mainly about first x single and about public x private apis. If my client (UI) sends me a user = 123, I shouldn't have a query that returns nullable, I want the error to happen.. and investigate why my UI is sending me a user that doesn't even exist..

Great showcase. Indeed the moment the Option is applied to the Data Access class, it ripples out clearing all of the ugly null check codes across the code base. What is the Nuget Package that provides you the Option class? Is it LanguageExt.Core? Because when I use it, I just need to change the return type and there should be no need to have check result is null in the return like at 5:30. For example: Before using Option: public T Load(Guid id, bool includeSoftDeleted = false) => LoadMany(id.PutInList(), includeSoftDeleted).FirstOrDefault(); After applying Option (just need change the return type of the method signature, the method's body is not changed): public Option Load(Guid id, bool includeSoftDeleted = false) => LoadMany(id.PutInList(), includeSoftDeleted).FirstOrDefault(); Not sure if it is a different story for other libraries, or it is not applicable when querying data async (my example is non-async query using Dapper).

Hi Derek. First, (or single :) ), thank you for you content, it's really valuable. Maybe obe thing to tale in considerarion is performance when single vs firstordefault, specially when query vs non-indexes field

The exception mentioned at 4:40 could also be generated in a multi-user scenario where data is deleted by another user and the UI state of all other viewers of the app is now out of date. Clicking on the link to view the details of that item would cause the exception to be thrown.

Instead of returning null, how about returning an empty record where you only have to check the ID of that record returned to see if it's valid or a -1?

Begineer here with not yet a clear vision of Clean Architecture and DDD (not even half way on my first reading of the blue book) but there is something I am asking myself lately. Is it possible to apply Clean Architecture & DDD when you have an existing database (assuming I can't touch that)? For example when using EF Core DbFirst with scaffolding. I would probably keep the scaffolded models in the Infrastructure layer as part of DAL, but I am not sure on how to take advantage of EF entities tracking for my commands. Let's say I have my domain aggregate which on the existing database is splitted on many tables, I would have a repository which will allow me to retrieve and save my aggregate without knowing the underlaying implementation being splitted on many DAL entities. But how could I create an instance of the domain model and expose the required methods to manipulate the underlaying DAL entities in such implementation considering my domain doesn't depend on the DAL at all?. My first idea was to have the domain model depend on some interfaces (exposed by the domain) and let the scaffolded models implement them (taking advantage of them being generated as partial classes). I like this idea but seems to add more complexity just to avoid having the domain depending on the DAL. I would like to have someone's opinion on this, thanks

One use case for a 404 (and logging the missing id) even in an internal app would be to find the actual id the data access code tried to search for. If it is not some trivial "get 1 order" call, seeing the id can help debug the issue. Wrong url generation on a SPA client? Or bad logic? Or just a typo at a different piece of code? You are half step ahead, if you don't have to grab a debugger to get the missing id. Might not even be an option if the error is in the production system. Not like anybody has errors in prod, since specs are complete, perfect, and all cases are covered by enough tests :)

Interesting video. Comming from php/Laravel point, I love the fact, that they've added firstOrFail() method in the ORM, so it handles those cases in nice way. For MVC projects its fine as you said, but those exceptions might actually be useful if you need some domain layer to control the business logic.

Couldn't agree more, however on point of using SingleAsync, I think it does a top 2 query which could potentially be extra load on the db? why not FirstAsync 😁 also on in an eventual consistence env I would argue returning 404 is actually dangerous, and a 500 can actually signal the caller to retry later perhaps

Thank you, but my opinion is the original approach is little better , persistent layer should not throw exception, low layers should return optionals not throw exception while higher layers can decide what they can do with it throw exception or handle the situation

Please do more like this

Think that this could be simplified by only using SingleOrDefault, and not having to use the Option. Controller would check the result from the caller whether it is null or not. If it is null, then return an IActionResult of NotFound and if something was found, then return Ok with the resource that was found as the body.

Thanks. Insightful and Succinct.

For me, there is no reason to define my own exception types. All exceptions I need are already there. Do you see any uses cases where you need your own ones?

I kinda do not understand what is the bonus of switching from nullables to options here :-)

Yes! Error as a value all day long, thank you.

I thought that Nullable was already C#'s version of Optional

I tried looking into event store db a few times and every time I found their site confusing

You don't control the route. All a user would have to do is change the URL in their address bar and suddenly your API is receiving an ID for an entity that doesn't exist. You account for that by returning a 404, not by letting LINQ throw an exception for you. Jeez.

well in performance point of view First() will return the first matched condition and single() go through all the list

Well, i think you´ll have to make a video called: "Single x First in depth" :)

Agree with everything except SingleOrDefault(). Please don’t use it, it forces SELECT TOP 2 in database if you’re using it in EF/EFCore. For the love of god, don’t read more data than required.

Why would you choose `Option` instead of nullable type and not just check for null to return `NotFound`? Looking from Kotlin it looks smelly.

you enjoy the if/else pretty much

Why use "Option" and not "Nullable" all the way? Same thing

Potential 404 or 403 or 401 should be handled in request validation or middleware, it should never reach your internal code

I see that misuse of First vs. Single all the time. And I cringed at that and the using exceptions for flow control. I always return a 404 and not a 500 (their fault, not mine, for asking for something that doesn't exist) but I can see your point on the 500, especially for something that is meant to be a permanent resource (perhaps they bookmarked it?). Still seems a bit off, but it could also be a sign that someone is hacking URLs in an attempt to see if you failed at the authn/authz (insecure direct object reference?). On a public API, I have returned a 403 in place of a 404 in some cases. If they are looking for the order of another customer, for example, I don't even want to tell them it doesn't exist. None of their business.

Nit picky but makes sense. I usually do it howbyou initially had it coded, for pre-emtpvie safety.

I won't complain like the other guys in the comments because the throw excepcion thing is a bad practice that we should AVOID. Period.

Good one.

This is a scenario where I actually like checked exceptions in Java.

what language is that?

titan submersible has thrown an exception derek

tl;dr: EAFP instead of LBYL.

More more moreee

جميل جدا جدا جدا

Promo-SM

I'm new to EFCore and was just scratching my head about this today, so thanks for this!