# View model with properties that represent years and months

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?

-
You don't need to order anything here, .Reverse() is simpler and more efficient. Also, do you really want to have the months backwards, or is that a copy & paste error? –  svick Jan 23 '13 at 19:29
thanks @svick! I really do not need orderByDescending here)) –  Aleksei Chepovoi Jan 24 '13 at 8:26

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.

-
This feels like too much sharing for the format part to me. Just because you know that years are always going to have at least 4 digits doesn't mean you should use the 00 format for them. –  svick Jan 23 '13 at 19:28

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.

-

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).

-
thanks! it's a great insight to use DateTime.Today.Year; –  Aleksei Chepovoi Feb 5 '13 at 10:40