Video není dostupné.
Omlouváme se.
Code Review & Refactoring to a better design
Vložit
- čas přidán 30. 07. 2024
- 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
eventsto.re/codeopinion
🔔 Subscribe: / @codeopinion
💥 Join this channel to get access to a private Discord Server and any source code in my videos.
🔥 Join via Patreon
/ codeopinion
✔️ Join via CZcams
/ @codeopinion
📝 Blog: codeopinion.com
👋 Twitter: / codeopinion
✨ LinkedIn: / dcomartin
📧 Weekly Updates: mailchi.mp/63c7a0b3ff38/codeo...
(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.
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.
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 basically a question of how your app recovers from exceptions, generally speaking. Often there's very little you can do if data goes missing.
This video is ignoring the point of status codes, you might want a 404, a 503, or even a 504. I feel like he is saying you just need to know if it’s there or not but that’s not enough. I can tell he doesn’t have api experience because he uses 400 and 404 interchangeably when those codes are generated at complete opposite ends of the stack. You should also not need to rely on a 500 to log, again 500 is “unknown”, codes exist for a reason. This might make the code prettier but debugging will be worse.
It's such a relief to hear these opinions that i completely agree with.
Hearing opinions I completely agree with makes me concerned I'm in an echo chamber.
@@gJoniiikr.. I'm always concerned whenever I feel like I'm experiencing confirmation bias
Hope you do loads more code review videos! I could definitely do with fixing some bad habits
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 agree with this, I think this concept is starting to catch on, but old habits die hard.
I think the issue is that BCL historically didn't have proper return types like Option, Either and so on. So if I didn't have the value expected by the return type, I had no other option but to throw or return null. You could develop our own implementations or use existing 3rd party libraries, but the industry as a whole won't adapt it.
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.
from my current experience, using the Option/Maybe pattern works really well when coupled with functional/rail-way oriented programming. when using them in an imperative manner you just end up with verbose checks everywhere when using such methods.
That's definitely a big downside of using this pattern. However, at least it tells developers to run these checks and allows you to fail gracefully without a performance hit or overloading your exceptions with data.
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.
There are some issues with global exception handling - firstly, you let the thing bubble and that's, unfortunately, costly as the stack trace has to be rebuilt multiple times. Secondly, you hide the logic of handling specific exceptions, which makes it harder for less talented/experienced devs to follow. Thirdly, why are you throwing an exception? It's heavier than Result/Option objects. Is it specific to the workflow? Then fail gracefully. Is it more generic? Then let things burn, imo. An angry user will force your managers to let you fix it :) I've recently written a small app - 6 endpoints and 3 Azure Functions - maybe 2-3 thousands lines of code. This entire app throws a total of 3 exceptions. All of these in cases that could only happen due to a blatant dev mistake.
@@Fafix666 there are some issues with any approach if devs don't understand the arch and strategies applied in the project they are working on. When using business exceptions and global handling correctly you won't suffer the issues you've mentioned, likewise you would suffer similar issues If you apply Option incorrectly. I'm not favoring one over the other, it's just that seems incorrect to disparage a proven pattern in OOP to present an alternative from functional programming.
The guy in the video was mixing business exceptions with worflow control, so it was a bad implementation.
@@juanprosalesmodern programming is an amalgamation of functional and objective. These two approaches are good for different things. Just because it's functional, doesn't mean it's not a valid alternative.
Devs understanding the arch will not make Exceptions work differently. One of the most important exception handling rules is handling them as soon as you can, this proven pattern directly breaks this principle.
This said, global handling is a good backup to fail gracefully in case of unexpected exceptions, but nothing more, imo.
@@Fafix666 these two approaches are good for different things, agree, I didn't mean is not valid bc is functional. My point is that code reviews should show devs what is wrong and how to do it well, in the approach they are using first, before switching to a new alternative. We can't trust that a new alternative would fix developer's bad habits, we would find in code reviews creative ways of using discriminated unions incorrectly too 😀.
Can you please talk about the option type?
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.
We use a very similar pattern, developed over a long time of suffering, and it has worked out really well for us ever since!
why use Option type when the compiler can warn you when you are accessing a member of a nullable value?
I'm new to EFCore and was just scratching my head about this today, so thanks for this!
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.
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?
Depends on the usage of the underlying provider. There will be a performance hit using Single() if it's in memory, but that's a trade-off you'd need to consider depending on your context and what the performance hit is.
Ah I just commented basically the same before reading through the rest of the comments. This is indeed a consideration if the performance loss trade off is worth making when using Single.
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.
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.
Option can be a bit jank in certain situations but I'd still prefer it over a nullable in various contexts (one being legacy where you're trying to migrate to nullable reference types). You can also add some useful extensions on top of it and implicit operators.
@@CodeOpinionwhy would you prefer it? What are the concrete benefits?
@@M0ns1gn0r the concrete benefit is that you are forced to handle it for the compiler to work, Nullable the compiler does not force you do handle.
@@gardnerjens it does
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.
Would love to see a video of you refactoring this further to remove the indirection created by the IOrderReadService
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
Depends on the underlying provider and what the performance hit is. Context matters. I generally want to know when there underlying data isn't as expected.
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?
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.
Is it happened with .NET and C# as well that moads are now the part of the infrastructure?
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.
meaning something like this: `public Task Handle(GetOrderQuery request, CancellationToken cancellationToken) => _orderService.GetOrderByIdAsync(request.OrderId);`
I personally would not use the arrow function here because it has become less readable.
Reasons:
- It is a long string and hard to read even if it is split on two lines or more.
- in the future this method can be extended and you have to switch to the regular method style.
- other methods can be more complicated, so you will have different styles in different handlers which is confusing.
I would use arrow functions if it is really short like simple properties.
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!
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.
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.
In my opinion, I would lean towards it being handled by the handler. I don't think the database access code should care about how to handle a null situation, it should just return null then the handler decides how to handle that situation.
Thanks. Insightful and Succinct.
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 😊
Well, First may still mask the problem. Single will not, however, I find "Find" (heh), while faster, to have its own problems - often, you don't want to return your entire db entity, but rather a thinner version of that entity (removing audit columns like CreatedBy for example). In this case, "Find" will first force you to fetch the entire thing and only then map. That's generally considered an anti-pattern as you are allocating more heap space than necessary and slowing down the response. Whether this calculates for you, depends on your specific use case, I think.
@@Fafix666 but there will never be a problem when the primary key is unique. A SQL db will never let you define two equal keys when the column is unique, and primary keys always are. Agree on the other part, didn't even consider that 😬
@@moellerdk93 that's true, but it may not be a PK that we're searching after. But I didn't see that part in your message.
In addition to the retrieving of the entire record, find also can not do an include to retrieve associated data in a single request.
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?
You're talking about two different things, collections in memory and a database query. If you call Single() against a DB provider and it behind the seems does a LIMIT (or TOP) 2, and you get one record back to memory. First() and Single() in this case are the same perf wise. At worst you have duplicate records and Single() on the DB actually returns 2 records. Which would then indicate you have bad data.. First() will then mask this issue.
@@CodeOpinion Single is minimally slower and a bit more chatty. But, I always prefer single to first when one record on no records is expected. Of course, this is an EF thing.
I used to use Find() but the EF team recommends against it and keeps it for backward compatibility reasons.
I tried looking into event store db a few times and every time I found their site confusing
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
Don't do it. In some cases ddd is not needed and will overcomplicate stuff.
hey dumb question; since I am a book freak. The bottom book is definetely DDD by Eric Evans, but what's on top??
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).
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.
Please do more like this
This is an approach I've commonly seen and applied with spring boot
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.
No. If it’s using the primary key lookup it will fetch a single record. It won’t scan the table.
@@allyourfuturebelongstochinasame with unique indexes.
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
Sure good suggestion.
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?
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..
Exactly. You'd be masking the problem of how you're getting bad data to begin with. If you were going to be defensive you'd probably also want to be logging whenever that occured.
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.
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
Yes, Single() does a TOP/LIMIT 2. If you're querying a primary key you're going to get one record back. No perf difference. If you're querying a clustered index, getting 2 records back is negligible. If you're using no index, well you might bigger problems in general depending on the size of date. So why not just use First(), because you're expecting one record, and if there is more than one you're masking data issues.
Yes! Error as a value all day long, thank you.
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.
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 :)
Good one.
I kinda do not understand what is the bonus of switching from nullables to options here :-)
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.
Sure, the record might not exist. So how you want to handle that back at the top level to provide something meaningful to the client. Generally don't think about data not existing in that sense because in my context it's never really deleted. Eg, an order is cancelled, product is discontinued, etc.
@@CodeOpinion Agreed - the "literally deleted from the database" part is the least interesting aspect of this. What matters is that the business needs were modeled and represented in code.
Things to consider:
- The UX and workflow of the person using the system. A raw 404 response isn't helpful UX.
- The information displayed to help the person take the next productive step. If there is an error do they know what step to take to resolve it.
- How the different scenarios are handled in the application code by other components. Should a high volume of "not found" over a short period of time result in a notification to administrators, even if the person using the UI can still load the page? (e.g. a "soft error")
@@CodeOpinion The client may want to know the difference between the order not existing and the system is down. For a resilient client, you may want to retry the API call with jittered delay on a 500, but a 404 is definitive answer and not to retry.
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?
ErrorOr is interesting in that you use custom exceptions but you don't "throw" them... you return them and have the same info in the caller (or pipeline behavior, or pipeline filter, etc.) that you would if it was thrown without having to catch it.
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.
I think that depends on the situation. If this is production code I would say put the null handling in the handler as that is business logic, and we want to encourage future programmers to code in a way that keeps the code easy to maintain.
@@evancombs5159 I would agree it depends. The way that I have seen APIs built, null is acceptable to return from the service method to the controller
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.
Why would you choose `Option` instead of nullable type and not just check for null to return `NotFound`?
Looking from Kotlin it looks smelly.
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
I thought that Nullable was already C#'s version of Optional
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.
Is it safe to assume you didn't watch/listen to the entire video?
you enjoy the if/else pretty much
Well, i think you´ll have to make a video called: "Single x First in depth"
:)
what language is that?
well in performance point of view First() will return the first matched condition and single() go through all the list
It's being done by the database. It's not an in memory list.
@@CodeOpinion In case of a query that is translated to SQL, the First() will translate to a SELECT TOP 1 and Single will translate to SELECT TOP 2, it up to you which one you select but i always use FirstOrDefault()
@@faisal3374 Nope. Using TOP 1 / 2 wont make a difference.
Why use "Option" and not "Nullable" all the way? Same thing
More more moreee
Nit picky but makes sense. I usually do it howbyou initially had it coded, for pre-emtpvie safety.
In a single example or in a very small app, yes I can agree it's nit picky. But in a large system this becomes death by a thousand papercuts.
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.
And I log all 401/403s...
i have an issue with returning non 200er Return Codes. How to differentiate between an 404 (Object not found) and an 404 Server not found? but i understand you security idea (hidding of endpoints in case of searching the wrong data)
@@markusschmidt9425 Hi! 404 is the URL (resource), not the server. From RFC 2616:, on 404: "The server has not found anything matching the Request-URI." Not finding the server wouldn't be an HTTP status code - it would be a DNS error, right? I'm confused - if they couldn't find your server, how would you send a 200?
Potential 404 or 403 or 401 should be handled in request validation or middleware, it should never reach your internal code
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.
For the love of god, if you only have one record that matches your condition, you'll only fetch one record. You won't be reading more data than required.
There is a slight performance using Single vs First. With First, the database will stop searching as soon as it it gets a hit. With Single, it has to continue reading until it either gets a 2nd hit or it is satified there are no other hits. The SQL is doing a SELECT TOP 2, but whenever everything is normal then it will only be returing 1 row. It will only return 2 rows if there is a problem. So where is the performance hit? - It is database side when it continues looking for that 2nd hit. If it is using indexes for finding the hits then this is negligable overhead. If it is not using indexes then it will have to sequentially scan the entire table. But if that is happening then you have a bigger performance problem than just the Single v First issue.
@@CodeOpinion If there is only one record why don't you use FirstAsync?
@@steve-wright-uk in the context of MSSQL, in most cases, I'd still take the readable intent of only a .Single record being fetched and a potential to catch data inconsistency over db performance hit due to a missing index, not saying .First is a no-no, but more like an exception, imho
@@ConstantinStoica Because if you have bad data it will mask it. Not everything can be enforced by a unique constraint at the db level. Example, lets say you have an active (boolean) column and you only ever want one record to be true. But many records can be false. You can't enforce that constraint at the db level. You'd have to make the column nullable and treat null as false, which is going down a crazy road.
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.
I would argue that Single still is a (tiny) performance impact even on databases, because EF Core, for example, will query for 2 rows and then only return 1, compared to First where it'll query for 1 row and return that 1 row.
single can be slow if there is not index , first will search for first match but single can search for the second, if the second is not there it will scan whole table, but anyway without index first will be slow also but single will be slower
@@victor1882 Correct. However if you're expectation is there should only be one row this should only occur if you have bad data. And that's the point is Single() will tell you you have bad data. First() will mask it.
@@CodeOpinion but thinking from the other side now, if I know I don't have bad data because of database constraints, isn't First just free performance on the account of a semantic difference?
@@victor1882 I agree this follows the same logic that was pointed out with the null return and throwing an exception. So I would think First is technically the correct thing to do. There is no need for the get to validate the database.
tl;dr: EAFP instead of LBYL.
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.
This is a scenario where I actually like checked exceptions in Java.
جميل جدا جدا جدا
titan submersible has thrown an exception derek
Promo-SM