snippetcsharpModerate
Enumerate list with filter criteria
Viewed 0 times
criteriawithfilterlistenumerate
Problem
Given the following
Is LINQ a better performer?
CreditCard object, how would you get a count of unexpired credit cards? What are the pros/cons of using each method? Or is there a better/optimal way?public class CreditCard
{
public int Year { get; set; }
public int Month { get; set; }
public int Day { get; set; }
}
public class User
{
public User()
{
CreditCards = new List();
}
public List CreditCards { get; set; }
public int NumberOfCreditCards
{
get
{
int count = 0;
DateTime localDate = DateTime.Now;
foreach (CreditCard creditCard in CreditCards)
{
DateTime expDate = new DateTime(creditCard.Year, creditCard.Month, creditCard.Day);
if (DateTime.Compare(expDate, localDate) > 0)
{
count++;
}
}
return count;
}
}
}Is LINQ a better performer?
public int NumberOfCreditCards
{
get
{
DateTime localDate = DateTime.Now;
return CreditCards.Select(creditCard => new DateTime(creditCard.Year, creditCard.Month, creditCard.Day))
.Count(expDate => DateTime.Compare(expDate, localDate) > 0);
}
}Solution
I will assume that the
With that said, first of all, I wouldn't use
More things that I'd change:
CreditCard class is set in stone, otherwise I probably would store a DateTime in it and calculate Year, Month and Day properties based on that value. Or at least add a separate method or property that'd return a DateTime.With that said, first of all, I wouldn't use
DateTime.Compare since there're documented operators for comparing DateTimes (see further here). Using them is certainely more readable, because reading DateTime.Compare(d1, d2) > 0 makes one (at least me) lag for a moment to realize what exactly it means. Personally I like the LINQ variant much more than the loop one, but I don't think that the intermediate Select statement is necessary, because it contains such trivial logic. I think that the most readable variant, that I'd probably put into production code, looks like this:public int NumberOfCreditCards
{
get
{
DateTime now = DateTime.Now;
return CreditCards
.Count(card => new DateTime(card.Year, card.Month, card.Day) > now);
}
}More things that I'd change:
- It's probably debatable, but I feel that making
NumberOfCreditCardsa method and renaming it into something more transparent likeGetNumberOfActiveCreditCardswould make the code cleaner. It depends on current time and doesn't seem (at least to me) to be a logical atribute of theUsertype (official guidelines).
- You should consider defining the public
CreditCardsproperty asIReadOnlyCollection. AListshould be exposed only when you expect the client code to modify your collection, otherwise it's better to use a restrictive interface. AndIReadOnlyCollectionis better thanIEnumerablebecause it clearly states that the property returns a collection that won't be enumerated at runtime (e.g. that it's not a method withyield return;). It's quite a common practice and the reasoning is written by the TS here. But again, this topic is debatable.
Code Snippets
public int NumberOfCreditCards
{
get
{
DateTime now = DateTime.Now;
return CreditCards
.Count(card => new DateTime(card.Year, card.Month, card.Day) > now);
}
}Context
StackExchange Code Review Q#112522, answer score: 11
Revisions (0)
No revisions yet.