Delete records with multiple ids based on condition

Question

Below is my class :

 public partial class Ads
    {
       public int Id { get; set; }
       public int RegionId { get; set; }
       public string Name { get; set; }
       public int Group { get; set; }
    }

Records :

Id      Name   Group
1       abc     1
2       xyz     1
3       lmn     1
4       xxx     2
5       ppp     2
6       ttt     3
7       ggg     3

Now I want to remove all records/only that record with particular id of same group for some ids.

Code :

public void Delete(int[] ids,bool flag = false)
        {
            using (var context = new MyEntities())
            {
                context.Ads.RemoveRange(
                    context.Ads.Where(t => (flag ?
                   (context.Ads.Any(x => ids.Contains(x.Id) && x.Group == t.Group)) : false)));
                context.SaveChanges();
            }
        }

What I am trying to do is something like below :

If flag is false with ids=3,5 then
    I want to delete only records with Id=3,5
Else if flag is true with ids=3,5 then
    I want to delete records with Id=3,5 but all other records too of the group to which ids=3,5 belong to.
    Here id=3 belongs to group 1 so I want to delete all records of group1 i.e id=1,2 like wise ids=5 belongs to
    group 2 so I want to delete all records of group 2 i.e id=4.

Expected output for this last case(flag=true) :

Id      Name   Group
6       ttt     3
7       ggg     3

But I think that I haven't done this is a proper way, and there is some source of improvement in the query.

Note : ids[] will always contains ids from different group and that too highest ids from different group.

How can I to improve my query for both the cases(flag=true and false)?

Hi Learning. Please recall the advice [I provided before](https://stackoverflow.com/questions/46450952/how-to-delete-child-automatically-based-on-parent-deletion-for-database-first-ap#comment79865111_46450952), which was: _when referring to yourself, please always use a capital "I". Native English readers generally find it rather grating to read "i", and that minor frustration response can get in the way of assisting._ Would you remember to resolve that on your next post, please? — halfer, Oct 13 '17 at 18:48

jitender · Answer 1 · 2017-10-12T07:08:44.947

2

What about

var removeRecs=context.Ads.where(t => ids.contains(t.id))
if(flag)
removeRecs.AddRange(context.Ads.where(t=> removeRecs.Any(r =>t.groupId==r.Id)))
Ads.RemoveRange(removeRecs);

edited Oct 12 '17 at 07:08

answered Oct 11 '17 at 12:11

jitender

10,238
1
18
44

I am not getting this addRange method – I Love Stackoverflow Oct 12 '17 at 06:46
it should be [AddRange](https://msdn.microsoft.com/en-us/library/z883w3dc(v=vs.110).aspx) – jitender Oct 12 '17 at 07:08
Its not about syntax problem but i am not getting that AddRange method. – I Love Stackoverflow Oct 12 '17 at 08:08
try removeRecs.ToList().AddRange bcs AddRange will only be available on IEnumerable instead of Iquariable – jitender Oct 12 '17 at 08:49

score 1 · Answer 2 · answered Oct 11 '17 at 12:11

Do not make it too hard for your self, not everything must/can be done in the where statement of a query. Also a general rule of thumb in a loop try to factor out all the constant values and checks. So try this:

    public static void Delete(int[] ids, bool flag = false)
    {
        using (var context = new MyEntities())
        {
            var query = context.Ads.AsQueryable();
            query = flag
              ? query.Where(x => context.Ads
                                   .Where(i => ids.Contains(i.Id))
                                   .Select(i => i.Group)
                                   .Contains(x.Group))
              : query.Where(x => ids.Contains(x.Id));

            context.Ads.RemoveRange(query);
            context.SaveChanges();
        }
    }

Can you tell me something about this line :var query = context.Ads.AsQueryable(); — I Love Stackoverflow, Oct 12 '17 at 07:07
Perhaps the AsQueryable can be replaced by a cast like 'as IQueryable'. Or instead of using 'var query = ...' you could use 'IQueryable query = ...'. The goal is to make sure that the next statement can reuse the same variable. As this statement returns an IQueryable we can not start with the DbSet as type. How you write it is up to you and depends on how readable you want your code. In the end the compiler will optimize this statement. — verbedr, Oct 16 '17 at 20:03

grek40 · Answer 3 · 2017-10-11T12:30:02.830

1

You should separate your tasks...

if (flag)
{
    groupIds = db.Ads.Where(x => ids.Contains(x.Id)).Select(x => x.Group).ToList();
    db.Ads.RemoveRange(db.Ads.Where(x => groupIds.Contains(x.Group)).ToList());
}
else
{
    db.Ads.RemoveRange(db.Ads.Where(x => ids.Contains(x.Id)).ToList());
}

edited Oct 11 '17 at 12:30

answered Oct 11 '17 at 12:12

grek40

13,113
1
24
50

Why you are using distinct in your query? – I Love Stackoverflow Oct 11 '17 at 12:19
@Learning lets assume `ids` contains 1000 ids that belong to the same groupId. Why would I keep duplicates in the `groupIds`? – grek40 Oct 11 '17 at 12:20
This wont be the case like ids will always contains ids from different group only and that too highest ids from each of the group – I Love Stackoverflow Oct 11 '17 at 12:22
@Learning if you don't write it in your question, information doesn't exist to us who try to answer. Your question doesn't state this restriction, so I didn't assume it – grek40 Oct 11 '17 at 12:23
I am sorry for this.Let me add this information in my question.Thanks :) – I Love Stackoverflow Oct 11 '17 at 12:24
@Learning ok, removed the `Distinct`, but other than that, my suggestion stays the same... separate the cases and pre-select the groupIds in case of flag. – grek40 Oct 11 '17 at 12:31

Lucian Bumb · Answer 4 · 2017-10-11T12:24:50.877

 public void Delete(int[] ids, bool flag = false)
        {
            using (var context = new MyEntities())
            {
                var items = context.Ads.Where(x => ids.Any(a => x.Id == a));


                if (!flag)
                {
                    //flag=false --> delete items with Id in ids[]
                    context.Ads.RemoveRange(items);
                }
                else
                {
                    var groups = items.GroupBy(a => a.Group).Select(a => a.Key);

                    //flag=true --> delete all items in selected groups
                    context.Ads.RemoveRange(context.Ads.Where(x => groups.Any(a => x.Group == a)));
                }
                context.SaveChanges();
            }

You should include that group line in else part because when flag will be false that line will execute unnecessarily. — I Love Stackoverflow, Oct 11 '17 at 12:21

Robert · Answer 5 · 2017-10-12T16:49:08.983

To me it looks like you have two different deletes here.

In first case you are only deleting the ads with given ID and this is pretty straight forward.

In second case you are deleting the ads with given ID and all other ads that contain the group of the recently deleted Ads. So in this case instead of deleting the ads with given Id first why not actualy get distinct groups for these ID-s and than just delete the groups.

EDIT

You can do it like this.

using (var context = new TestEntities())
{
    if (!flag)
        context.Ads.RemoveRange(context.Ads.Where(a => ids.Contains(a.Id)));
    else
        context.Ads.RemoveRange(context.Ads.Where(a => context.Ads.Where(g => ids.Contains(g.Id)).Select(x => x.Group).Distinct().Contains(a.Group)));
    context.SaveChanges();
}

For the more complicated case I am trying to get distinct groups for given id-s. So for ID-s 3 and 5 I am selecting the groups and than I am doing distinct on the groups since it might happen that the id-s have the same group. Than I am fetching all the ads that have these groups. So for passed values of 3 and 5 I would get groups 1 and 2 which I would than use to get all the ads that have that group. That in turn would yield id-s 1, 2, 3, 4, and 5 which I would than delete.

EDIT 2

If the complexity of second Linq query bothers you than write a SQL query.

context.Database.ExecuteSqlCommand( 
        "DELETE Ads WHERE Group IN (SELECT Group FROM Ads WHERE Id IN(@p1, @p2))", new SqlParameter("@p1", ids[0]), new SqlParameter("@p2", ids[1]));

This should be extra performant rather than rely on EF which will delete it one by one.

Upvoted for your kind efforts towards helping me but dont you think less linq functions is equal to more efficiency — I Love Stackoverflow, Oct 12 '17 at 13:03
That rule applies everywhere - less functions = more efficency. The pseudo code I described is pretty simple it linq that lacks simplicity. — Robert, Oct 12 '17 at 16:42

Delete records with multiple ids based on condition

5 Answers5

Linked