Better way to do a SelectAll, Filter, Update (on all results)

Question

So I have this piece of code which I think can be improved but I don't know how

IList<User> users = await _unitOfWork.UserRepository.SelectAllAsync();
users = users.Where(x => x.Field == MyField).ToList();
foreach (var user in users)
{
   user.Active = IsActive;
   await _unitOfWork.UserRepository.UpdateAsync(user);
}

What I want to achieve is:

1) get all entries
2) filter them
3) on the filtered list, update their active status

How can I improve this code, both performance-wise and clean-wise?

Thanks.

Why don’t you filter them when you get them? – stuartd Nov 26 '22 at 22:28 — stuartd, Nov 26 '22 at 22:28
And what is `_unitOfWork` doing? – stuartd Nov 26 '22 at 22:32 — stuartd, Nov 26 '22 at 22:32

score 5 · Accepted Answer · edited Dec 01 '22 at 05:53

No idea what's inside your _unitOfWork object, but the code seems to imply you are implementing the repository pattern. EF Core already implements that, so you are not achieving anything by adding another repository on top of it.

If you use a DbContext directly, you can improve your code as follows...

IList<User> users = await _ctx.Users
                              .Where(x => x.Field == MyField)
                              .ToListAsync();

foreach (var user in users)
{
   user.Active = IsActive;
}

await _ctx.SaveChangesAsync()

This has two advantages over your code:

Your code pulls all users out of the repository and then filters in memory. Depending on the number of users, this can be inefficient. My code only pulls out the users that you want.
Your code calls the database for every user. As my code is just updating the local context, it only needs to call the database once, when all the updates are done.

score 2 · Answer 2 · answered Nov 28 '22 at 02:24

Another option to consider is using EF Core 7.0 (EF7). EF7 targets .NET 6, so you can use it with either .NET 6 or .NET 7.

With EF7, you now have the ability to use bulk updates. This allows you to update your selected entities with a single command.

So your code could be written as follows:

await _ctx.Users
  .Where(u => u.Field == MyField)
  .ExecuteUpdateAsync(s => s.SetProperty(s => s.Active, s => IsActive));

The advantage of this approach is that your entities are not loaded into memory, so you can avoid having to use change tracking for updates. Depending on how many users you need to update, this can be much more efficient.

That being said, carefully consider using this method if you're relying upon change tracking in the rest of your app. None of your tracked entities will be kept in sync by default. The performance advantages of this approach are minimal if you're only updating a few users at a time.

score 0 · Answer 3 · edited Nov 30 '22 at 08:43

Your code is very memory and computationally inefficient. There are two reasons why.

Your code seems to use a repository pattern. My answer is based on that assumption.

First, the most problematic part is that you get all entries and then you select the records that match the where condition. With this pattern, you scan all records two times, but there is no need for that. Try to add a where clause in your parameters in your repository like below:

public async Task<List<T>> SelectAllAsync(Expression<Func<T, bool>>? filter = null)
{
    IQueryable<T> query = dbSet;
    if (filter != null)
    {
        // now you can put your where clause here
       query = query.Where(filter);
    }
    return await query.ToListAsync();
}

And when calling it you can simply do the following:

IList<User> users = await _unitOfWork.UserRepository.SelectAllAsync(filter: x => x.Field == MyField);

Using this method, you only scan your database once and you don't need to filter the result again.

Second, if you use this User table a lot to get all the Users based on the IsActive Column, add an index for it since it is inefficient to call this code more than necessary.

Better way to do a SelectAll, Filter, Update (on all results)

3 Answers3