HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcsharpModerate

Enumerate list with filter criteria

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
criteriawithfilterlistenumerate

Problem

Given the following 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 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 NumberOfCreditCards a method and renaming it into something more transparent like GetNumberOfActiveCreditCards would make the code cleaner. It depends on current time and doesn't seem (at least to me) to be a logical atribute of the User type (official guidelines).



  • You should consider defining the public CreditCards property as IReadOnlyCollection. A List should be exposed only when you expect the client code to modify your collection, otherwise it's better to use a restrictive interface. And IReadOnlyCollection is better than IEnumerable because it clearly states that the property returns a collection that won't be enumerated at runtime (e.g. that it's not a method with yield 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.