Code Review & Refactoring to a better design

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 that I didn't agree with around nullables and throwing exceptions. I'll show samples of what I'd prefer the API to look like and why.

🔗 EventStoreDB

💥 Join this channel to get access to a private Discord Server and any source code in my videos.

🔥 Join via Patreon

✔️ Join via YouTube

[philliphaydon]

(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.

[pilotboba]

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.

[MartinLiversage]

Option<T> 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<T> it doesn't really add any value in my opinion.

[MaxPicAxe]

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

[KennethBoneth]

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.

[buttforce]

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

[shadowsir]

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.

[AbramS]

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.

[VictorMachadoDeFranca]

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

[superpcstation]

Can you please talk about the option type?

[juanprosales]

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<T>. 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.

[Fafix]

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?

[cward]

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.

[ChallusMercer]

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?

[lluism]

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

[FlippieCoetser]

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.

[allinvanguard]

I'm curious, does Option<T> 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<T> 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.

[carmineos]

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

[buttforce]

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

[crazyfox]

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.