View model with properties that represent years and months

Question

I have a view model with properties that represent years and months:

public IEnumerable<SelectListItem> Years
{
    get
    {
        return new SelectList(
            Enumerable.Range(1900, 112)
            .OrderByDescending(year => year)
            .Select(year => new SelectListItem
            {
                Value = year.ToString(CultureInfo.InvariantCulture),
                Text = year.ToString(CultureInfo.InvariantCulture)
            }
        ), "Value", "Text");
    }
}

public IEnumerable<SelectListItem> Months
{
    get
    {
        return new SelectList(
            Enumerable.Range(1, 12)
            .OrderByDescending(month => month)
            .Select(month => new SelectListItem
            {
                Value = month.ToString(CultureInfo.InvariantCulture),
                Text = month < 10 ? string.Format("0{0}", month) : month.ToString(CultureInfo.InvariantCulture)
            }
        ), "Value", "Text");
    }
}

I definitely use copy & paste approach here. How can I refactor this code? Maybe somehow passing the numbers as parameters to some helper method?

Answer 1

You can use a format string to ensure that there are at least 2 digits in the format without having to check first. The format: {0:00} ensures there are at least 2 digits, but allows for more. It will prefix 0 to any single-digit input.

string.Format also has an overload which takes in an IFormatProvider, so my personal preference is just to collapse that into the string.Format call. It also happens to line up nicely :)

With that, you can create a method with appropriate parameters which can handle both cases:

  public IEnumerable<SelectListItem> Years2 { get { return BuildList (1900, 112); } }
  public IEnumerable<SelectListItem> Months2 { get { return BuildList (1, 12); } }

  public IEnumerable<SelectListItem> BuildList (int start, int count)
  {
     return new SelectList (
         Enumerable.Range (start, count)
         .OrderByDescending (val => val)
         .Select (val => new SelectListItem
         {
            Value = val.ToString (CultureInfo.InvariantCulture),
            Text = string.Format (CultureInfo.InvariantCulture, "{0:00}", val)
         }
     ), "Value", "Text");
  }

Note: I suffixed my properties with 2 so I could run them side-by-side and compare results to be sure I didn't miss anything.

Note2: as Jesse suggests, you should ideally cache the results.

Answer 2

You're over-creating these objects on each call. Cache static values:

    private static readonly IEnumerable<SelectListItem> years = new SelectList(
        Enumerable.Range(1900, 112)
        .OrderByDescending(year => year)
        .Select(year => new SelectListItem
        {
            Value = year.ToString(CultureInfo.InvariantCulture),
            Text = year.ToString(CultureInfo.InvariantCulture)
        }),
        "Value",
        "Text");

    private static readonly IEnumerable<SelectListItem> months = new SelectList(
        Enumerable.Range(1, 12)
        .OrderByDescending(month => month)
        .Select(month => new SelectListItem
        {
            Value = month.ToString(CultureInfo.InvariantCulture),
            Text = month < 10 ? string.Format("0{0}", month) : month.ToString(CultureInfo.InvariantCulture)
        }),
        "Value",
        "Text");

    public IEnumerable<SelectListItem> Years
    {
        get
        {
            return years;
        }
    }

    public IEnumerable<SelectListItem> Months
    {
        get
        {
            return months;
        }
    }

As for creating a helper method? I likely wouldn't. These mean two different things - years and months. Keep that logic separate.

Answer 3

I dislike the way you specify your year range, especially the upper bound. It looks like you want the current year as upper bound, and not a magic value of 112 years since the start.

112 is bad in two ways:

1. Its meaning isn't clear by itself, it only becomes the year 2011 when you look at the start value 1900
2. You probably need to update it every year. It seems unlikely that you want to have a constant upper bound of 2011 forever.

I'd use something like:

int startYear = 1900;
int endYear = DateTime.Today.Year;

var years = Enumerable.Range(StartYear, endYear - startYear + 1).Reverse();

Potentially factoring out the current date/year to a property of the current viewmodel or a service so your helper method doesn't access any external mutable state (DateTime.Today).