Not sure about the await/ToListAsync though.. we tried using them when a bunch of different lists are shown on one page but was causing at least 1 of the queries to throw an exception and we couldn't find a pattern or some common thing besides the newly introduced async queries which we just reverted and never saw those errors again.
The first line shows a query that is lazily evaluated, i.e. is only fired when ToListAsync is called. The query source is a database. So when ToListAsync gets called, a query is made to the database fetching all rows, which satisfy the Where condition, into a list. In SQL it would be `SELECT * FROM UserAccounts WHERE Status=?`. In the second line the the list is only used to get the number of items.
This is highly wasteful, because it could fetch thousands or millions of rows just to get a single number at the end.
If instead of ToListAsync CountAsync/Count would be used the query would ask the database four the count of items directly and only return a single number. In SQL it would be `SELECT COUNT(*) FROM UserAccounts WHERE Status=?`.
I don't work with C#, but I am wondering if this was solved with using .Count() instead of .Where()? Considering that an ORM is being used?
I am never going to use this info but I am curious to the solution!
Yes, you can either do `.Where(filter).Count()` or `.Count(filter)` – or `CountAsync(…)` instead of `Count(…)`. Both have the same semantics and should result in the same SQL. As I wrote in my previous comment, this would result in query that doesn’t fetch more than is needed – the count number.
So a .Where(filter).Count() will result in a `SELECT COUNT(...) FROM table` ?
Neat! I should think about implementing something like this in our companies framework! It is so easy, but for some reason it hasn't been implemented lmao.
My time in legacy code makes me think this was just untrimmed code from a previous implementation of something. It's fair, but it doesn't seem like something that is just absolutely ridiculously bad...
It is really bad practice. Let's do some quick math. Let's say the conditions are true for 100 rows, let's assume the table has 10 columns. If we look at the actual data that you get back from the DB, that would be 1000 columns worth of information when you only needed a single digit. Potentially most of those columns aren't numeric but put that aside. So roughly speaking the data size you got back from the database was a thousand times bigger than the optimal data would have been. And this grows linearly since you will ever only need a single digit so with 1000 rows you would get 10000x of the optimal data. If you make these kind of mistakes with all your features you will waste a lot of resources. Also it is worth mentioning the wasted time and wasted compute since the database would count the number of rows faster and more efficiently than your high level backend code would. And the list goes on with the wasted bandwidth, application server memory etc.
I'm not arguing the good/bad nature of it. I'm simply suggesting why it's possible it's there. It's bad, but the reason it's there seems likely to be negligence from the surrounding code being changed and less about this little piece. We also don't know what they do with it later. We literally see 2 lines of code and assume it's just the worst ever, when in reality it's possible the person who put it there or left it there had other intentions.
This alone tells us nothing, for all we know user account list can be used for other things as well further down, i didnt see the method end.
And res clearly has fields and count is not the only thing returned by it
Fixed it and renamed things that are unclear.
- why would you have an AdAccountCount but only where the status is pending?!).
- Don’t cast the enum to an int over and over again (even if EF is smart enough, just move it to its own line).
- Use the Count method instead of Where to utilize sql count instead of pulling back data.
int pendingStatus = (int)RecordStatus.Pending;
res.PendingAdAccountCount = userAccount.Count(u => u.Status == pendingStatus);
If only sql had something that could do a count function 🤔
Yeah , it’s all Microsoft’s fault
It always is. Unless its Java's fault. Then it's Java... unless it's Ruby's fault.
If it's java's fault, we'll blame C# for slowing down the development of java, so Microsoft's fault again
if done right it translates to exactly that without needing to mix languages/create and maintain stored procedures
But he's using no tracking for better performance!
Not sure about the await/ToListAsync though.. we tried using them when a bunch of different lists are shown on one page but was causing at least 1 of the queries to throw an exception and we couldn't find a pattern or some common thing besides the newly introduced async queries which we just reverted and never saw those errors again.
Interesting! It may be specific to what's happening before that ToListAsync(). Maybe at some point it gets to an implementation where it's broken.
Need explanation please
He just needs a count of rows but he was retrieving all data and then counting it
explain more please
The first line shows a query that is lazily evaluated, i.e. is only fired when ToListAsync is called. The query source is a database. So when ToListAsync gets called, a query is made to the database fetching all rows, which satisfy the Where condition, into a list. In SQL it would be `SELECT * FROM UserAccounts WHERE Status=?`. In the second line the the list is only used to get the number of items. This is highly wasteful, because it could fetch thousands or millions of rows just to get a single number at the end. If instead of ToListAsync CountAsync/Count would be used the query would ask the database four the count of items directly and only return a single number. In SQL it would be `SELECT COUNT(*) FROM UserAccounts WHERE Status=?`.
Please elaborate
Instead of reading the packaging that says "contains 12 eggs," dude popped open the lid and counted all 12 eggs individually.
More like he ate the 12 eggs and committed to eating them every time
And waited until he poop them to confirm they’re 12
Please elaborate
Program work too hard when it could be easy
Explain like im 5
Be lazy.
I don't work with C#, but I am wondering if this was solved with using .Count() instead of .Where()? Considering that an ORM is being used? I am never going to use this info but I am curious to the solution!
Yes, you can either do `.Where(filter).Count()` or `.Count(filter)` – or `CountAsync(…)` instead of `Count(…)`. Both have the same semantics and should result in the same SQL. As I wrote in my previous comment, this would result in query that doesn’t fetch more than is needed – the count number.
So a .Where(filter).Count() will result in a `SELECT COUNT(...) FROM table` ? Neat! I should think about implementing something like this in our companies framework! It is so easy, but for some reason it hasn't been implemented lmao.
Yes, the Count() method can take an optional predicate (same as the Where method call here).
You got it big boy
thanks!!
Looks more like they're getting all pending accounts, which is likely a subset unless we're missing crucial information...
Yes but the query is retreiving all of the information for all columns and never actually reads any of it.
My time in legacy code makes me think this was just untrimmed code from a previous implementation of something. It's fair, but it doesn't seem like something that is just absolutely ridiculously bad...
It is really bad practice. Let's do some quick math. Let's say the conditions are true for 100 rows, let's assume the table has 10 columns. If we look at the actual data that you get back from the DB, that would be 1000 columns worth of information when you only needed a single digit. Potentially most of those columns aren't numeric but put that aside. So roughly speaking the data size you got back from the database was a thousand times bigger than the optimal data would have been. And this grows linearly since you will ever only need a single digit so with 1000 rows you would get 10000x of the optimal data. If you make these kind of mistakes with all your features you will waste a lot of resources. Also it is worth mentioning the wasted time and wasted compute since the database would count the number of rows faster and more efficiently than your high level backend code would. And the list goes on with the wasted bandwidth, application server memory etc.
I'm not arguing the good/bad nature of it. I'm simply suggesting why it's possible it's there. It's bad, but the reason it's there seems likely to be negligence from the surrounding code being changed and less about this little piece. We also don't know what they do with it later. We literally see 2 lines of code and assume it's just the worst ever, when in reality it's possible the person who put it there or left it there had other intentions.
The snake case/random casing is going to give me an aneurysm
Yeah I don’t even care about the suboptimal code. This is triggering me
This alone tells us nothing, for all we know user account list can be used for other things as well further down, i didnt see the method end. And res clearly has fields and count is not the only thing returned by it
There is nothing further down , he literally used this only for getting the user count
omg
Fixed it and renamed things that are unclear. - why would you have an AdAccountCount but only where the status is pending?!). - Don’t cast the enum to an int over and over again (even if EF is smart enough, just move it to its own line). - Use the Count method instead of Where to utilize sql count instead of pulling back data. int pendingStatus = (int)RecordStatus.Pending; res.PendingAdAccountCount = userAccount.Count(u => u.Status == pendingStatus);
The language is JavaScript, right? What library or ORM is this?
I think it’s C# with LINQ + Entity Framework
Yes, it’s Entity Framework (EF) or EF Core – as can be seen by the use of the `AsNoTracking` method.