5
\$\begingroup\$

I know this is extremely trivial, but would you call the extension method below Remove or RemoveMany? Please provide justification.

public static void Remove<TEntity>(this DbSet<TEntity> set, IEnumerable<TEntity> entities)
    where TEntity : class
{
    foreach (var entity in entities)
        set.Remove(entity);
}

-- or --

public static void RemoveMany<TEntity>(...) { ... }
\$\endgroup\$
6
\$\begingroup\$

Your method is removing a collection of entities that exist in some set.

When I see Remove, I'd say it removes a single item from something. Now if that collection actually existed in the set, then it would be perfect, but alas it's a collection of items to remove.

One thing to consider, assuming you're making an extension method for the EF5 DbSet, it already has a Remove method that removes a single item, I think an overload to remove more items doesn't sound like a bad idea to me.

On the other hand, LINQ to XML defines a Remove() method on enumerables of XNode. But that's an extension on the collection of entities, not the parent set. I'm a little on the fence on this one.

RemoveMany, while better as it indicates we're removing many items, doesn't fit well with me. It mimics SelectMany by name but I'd associate the "Many" part with a projection, certainly not with a remove operation. I'd avoid that one.


I might consider ExceptEntities to be similar to the LINQ method Except. Except returns the set difference of two collections where the result contains unique values. Since this operation isn't quite the same, we'll still need to make this distinguishable hence the added "Entities". Since you are dealing with a set (?), this sounds perfect to me. But after thinking about this more, I'm not quite sure this is the better option since this modifies the collection.

I think RemoveEntities might be a better fit. "Remove" makes it sound as though we're modifying something. RemoveAll doesn't cut it for me as it is named the same as List.RemoveAll which removes all that matches a predicate. There's also a List.RemoveRange which removes a range of consecutive values in the collection, not quite the same.

So I'd go with RemoveEntities but an overload of the Remove name might be fine, just beware of the similarities of the names in other objects.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1. I think you managed to get down most of what I was debating in my head, plus a lot more :) Anyway, yes, I'm using the EF DbSet, and the extension method actually uses the built-in Remove() method inside the loop. As for RemoveEntities(), I see why you suggest this, but to me, it would only make sense if the native method were called RemoveEntity(). So, at the moment, I'm leaning toward just Remove() for the extension method. \$\endgroup\$ – devuxer Sep 2 '12 at 19:53
4
\$\begingroup\$

I'd consider calling it RemoveAll.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Interesting idea, but one possible problem with RemoveAll() is that it might imply that you are clearing out all the entities in the set. What this extension method is supposed to do is clear out only the entities that are passed into the entities parameter. \$\endgroup\$ – devuxer Sep 2 '12 at 19:07
  • \$\begingroup\$ @DanM I think this can be remedied by introducing an overload without the entities parameter, which would fit in well with the LINQ Any() and Any(x => x.Age > 18) overload scheme. \$\endgroup\$ – Adam Sep 2 '12 at 23:32
3
\$\begingroup\$

Just one correction. You cant modify the enumeration inside the for each. Try

    public static void RemoveMany<TEntity>(this DbSet<TEntity> thisDbSet, IEnumerable<TEntity> entities) where TEntity : class
    {
        for (int i = entities.Count() - 1; i >= 0; i--)
        {
            if (entities.ElementAt(i) != null)
                thisDbSet.Remove(entities.ElementAt(i));
        }

    }
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

How about RemoveExisting considering you are removing existing entities from the object.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'm removing only a subset of the existing entities, so I think this would be misleading. \$\endgroup\$ – devuxer Sep 2 '12 at 22:57
  • \$\begingroup\$ Fair enough. I thought you were removing all entities that were supplied in the parameter where they existed in the DbSet. \$\endgroup\$ – dreza Sep 2 '12 at 23:32
  • \$\begingroup\$ Wait, I actually am removing all entities supplied in the parameter, but I'm not removing all "existing" entities in the set. Wouldn't the method name would normally refer to the object being acted on (the DbSet) and not the parameter (the collection of entities)? \$\endgroup\$ – devuxer Sep 2 '12 at 23:36
  • \$\begingroup\$ yep, good point. \$\endgroup\$ – dreza Sep 3 '12 at 0:25
  • 2
    \$\begingroup\$ If you're removing a subset, why not RemoveSubset? \$\endgroup\$ – Glenn Rogers Sep 3 '12 at 18:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.