Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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");
    }
}

Definitely I use copy & paste approach here) How can I refactor this code? Mabby somehow passing the numbers as parameters to some helper method?

Edits:

Besides, I have other SelectListItem with strings, so I decided to create new static class with static method to handle both cases: strings and numbers. Here is my code: 

public static class SelectListHelper
{
    public static IEnumerable<SelectListItem> GetSelectList(IEnumerable<string> list)
    {
        return new SelectList(
            list.Select(i => new SelectListItem
            {
                Value = i,
                Text = i
            }
                ), "Value", "Text");
    }
}

And the code from model: 

private IEnumerable<string> _years = GetEnumerableRange(1900, 113);

private IEnumerable<string> GetEnumerableRange(int start, int count)
    {
        return Enumerable.Range(start, count)
                         .Reverse()
                         .Select(i => i.ToString(CultureInfo.InvariantCulture));
    }

So, now I can do the following:

public IEnumerable<SelectListItem> Years
    {
        get { return SelectListHelper.GetSelectList(_years); }
    }

public IEnumerable<SelectListItem> Colors
    {
        // _colors is of type List<string>
        get { return SelectListHelper.GetSelectList(_colors); }
    }

I decided to not give a f**k about {0:00} format)

share|improve this question
2  
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 at 19:29
 
thanks @svick! I really do not need orderByDescending here)) –  Aleksei Chepovoi Jan 24 at 8:26

3 Answers

up vote 2 down vote accepted

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.

share|improve this answer
 
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 at 19:28
up vote 2 down vote

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.

share|improve this answer
up vote 2 down vote

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

share|improve this answer
 
thanks! it's a great insight to use DateTime.Today.Year; –  Aleksei Chepovoi Feb 5 at 10:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.