Compare Collection with the information in DB using EF and LINQ

Question

I want to inform the Receivers about new contracts we have this week. After I sent the Emails, the Information about sent contracts are stored in MS SQL Database.

To avoid that someone become the same Email about contract more than one time I perform this steps:

1. I have the Collection with new contracts as the input
2. I read the information from DB using Entity Framework about sent Emails
3. If the Emails was already sent I add new Instance to the sent Emails Collection

After that, I remove the contracts from input collection that are already in sent collection:

public Collection<Contract> GiveContractsThatWereNotSent(Collection<Contract> newThisWeekContracts)
{
 var contractsWereSent = new Collection<Contract>();
 using (var accountingEntities = new AccountingEntities())
 {
 foreach (Contract newContract in newThisWeekContracts)
 {
 bool found = (from sent in accountingEntities .Tbl_SentProtocol
 where
 sent.CONTRACT_NO == newContract.ContractNo
 && sent.CONTRACT_NO_ALT == newContract.ContractNoAlt
 select sent ).Any();
 if (found)
 {
 contractsWereSent.Add(newContract);
 }
 }
 } 
 foreach (Contract contract in contractsWereSent)
 {
 newThisWeekContracts.Remove(contract);
 }
 return newThisWeekContracts; 
}

• Patryk Ćwiek: You're doing N queries to the database, where N = newThisWeekContracts.Count. Would probably be better [citation needed] to do just one query with two IN clauses. On the other hand, having two IN clauses with thousands of items each would probably be not so great either and would require further restructuring.

Answer 1

Several points:

• Write your code against abstractions. Collection is a specific implementation.
  • You don't need newThisWeekContracts to be specifically a Collection. I'd take in any IEnumerable<Contract> instead.
  • By returning a Collection, the caller can add to it, or remove from it, which probably isn't the intended behavior. I'd return an IEnumerable<Contract>, or, if adding/removing is ok, I'd return an IList<Contract> or <ICollection<Contract> (i.e. an abstraction, not a specific implementation).
• I like the descriptive naming, casing is correct and all, but the names read a little bit awkward still.

It looks like the number of queries can be reduced, haven't profiled this but I would think it consumes fewer resources; the main thing is combining .Where() with .Any() and !.Any() as needed:

public IEnumerable<Contract> GiveContractsThatWereNotSent(IEnumerable<Contract> newContracts)
{
 using (var entities = new AccountingEntities())
 {
 var sentContracts =
 entities.Tbl_SentProtocol
 .Where(sent => 
 newContracts.Any(contract => 
 sent.CONTRACT_NO == contract.ContractNo
 && sent.CONTRACT_NO_ALT == contract.ContractNoAlt))
 .ToList();
 }
 // ...
}

That gives you sent contracts (/ contractsWereSent), but that's not what you're after. Now you need to return the contracts in newContracts (/ newThisWeekContracts) that are not in sentContacts.

What you have here:

foreach (Contract contract in contractsWereSent)
{
 newThisWeekContracts.Remove(contract);
}

Involves calling EqualityComparer<T>.Default on each contract, which verifies whether Contract implements IEquatable<T>, otherwise uses overrides of .Equals and .GetHashCode if Contract overrides them, otherwise uses reference equality... which doesn't sound very neat, since if a contract is "equal" when its ContractNo and ContractNoAlt exists in sendContracts when comparing in Tbl_SentProtocol, then I'd use the same equality comparison when I want to "remove" items from the newContracts.

In fact, I wouldn't remove anything from newContracts. Keep inputs for inputs!

How about this?

IEnumerable<Contract> sentContracts;
using (var entities = new AccountingEntities())
{
 sentContracts = // trying to prevent horizontal scrolling...
 entities.Tbl_SentProtocol
 .Where(sent => 
 newContracts.Any(contract => 
 sent.CONTRACT_NO == contract.ContractNo
 && sent.CONTRACT_NO_ALT == contract.ContractNoAlt))
 .ToList(); 
}
return newContracts.Where(contract => !sentContracts.Any(sent =>
 sent.CONTRACT_NO == contract.ContractNo
 && sent.CONTRACT_NO_ALT == contract.ContractNoAlt))
 .ToList();

---

One more thing, the entities should be named so as to read like normal code - entity type SentContract can be mapped to table tbl_SentProtocol, and properties ContractNumber and ContractAltNumber can map to columns CONTRACT_NO and CONTRACT_NO_ALT ;)