Use temp variable or inline in this example?

Question

I'm loading an ASP.NET dropdown list.

Is there any advantage to doing this::

    Private Sub LoadSeasonsListbox(ByVal seasons As List(Of Season))
      Dim li As ListItem
      For Each s As Season In seasons
        li = New ListItem(s.SeasonDescription, s.SeasonCodeID)
        frm.SeasonsList.Items.Add(li)
      Next
    End Sub

over this:

Private Sub LoadSeasonsListbox(ByVal seasons As List(Of Season))
    For Each s As Season In seasons
        frm.SeasonsList.Items.Add(New ListItem(s.SeasonDescription, s.SeasonCodeID))
    Next
End Sub

Answer 1

I don't think there is any real performance difference between them, nor correctness difference.

I think the first one is easier to debug, because you can easily stop on the Add line and view the contents of li.

Answer 2

I think¹ that both will actualy compile to the same bytecode. However, I usually only tend to introduce new variables when I am going to use them. Or when it makes the code clearer. In this case the statement is hardly very complex and immediately visibly what you are doing, so there is probably no real benefit to using a temporary variable.

¹ It's just a guess. Verify with Reflector or similar.

Answer 3

When debugging, the first makes it easier to examine the ListItem being added.

The first also has a lower width which some may find easier to read (but a higher height which some may find harder to read...)

Answer 4

The only advantage of the seconf over the first is that it eliminates a variable which is only used once. Something that in refactoring is sometimes desirable.

On the other hand, using an intermediatory variable like this can make the code a little more readable by breaking the steps involved down a bit.

Answer 5

I would originally write it and debug using method 1, then refactor to method 2 once I have it working well with my sample data.

Answer 6

You can also do it using LINQ (and C# because I cannot write VB.NET code ;).

private void LoadSeasonsListbox(IEnumerable<Season> seasons)
{
    frm.SeasonsList.Items.AddRange(seasons
        .Select(s => new ListItem(s.SeasonDescription, s.SeasonCodeID))
        .ToArray());
}

I prefer this solution because I hate this loops just copying or translating objects - they clutter the code so much. I would even think about writing an extension method.

private void LoadSeasonsListbox(IEnumerable<Season> seasons)
{
    frm.SeasonsList.Items.AddRange(
        seasons.ToListItems(s => s.SeasonDescription, s => s.SeasonCodeID));
}

Answer 7

i think they are same but i prefer the latter