Optimising asp.net vb code behind

Question

I have the below code.. i am finding a way to optimise it..

is there anyway to optimise it using findcontrol and loops ?? also eliminating headofficeSelected,areaSelected ,headofficeSelected and only using storeSelected ?

If (storeSelected = 1) Then
    tempCollector = tempCollector + "<br>" + "Department" + ": " + txt_panview3_ddinput1.SelectedItem.ToString
    tempCollector = tempCollector + "<br>" + "Store" + ": " + txt_panview3_input2.Text + "<br>"
End If
If (areaSelected = 1) Then
    tempCollector = tempCollector + "<br>" + "Area Name" + ": " + txt_panview0_ddinput1.SelectedItem.ToString
End If
If (headofficeSelected = 1) Then
    tempCollector = tempCollector + "<br>" + "Department " + ": " + txt_panview1_ddinput1.SelectedItem.ToString
End If
If (regionSelected = 1) Then
    tempCollector = tempCollector + "<br>" + "Region " + ": " + txt_panview2_ddinput1.SelectedItem.ToString
End If
'
If storeSelected() = 0 Then
    tempCollector = tempCollector + "<p><b>" + lbl_viewTitle2.Text + "</b>"
    tempCollector = tempCollector + "<br>" + lbl_view2_ManagersEmailAddress.Text + ": " + txt_view2_ManagersEmailAddress.Text
End If

Answer 1

How are you trying to optimize it? Are you trying to make it run faster? If so have you profiled this to make sure it's the bottle nexk.

Are you trying to optimize the code to make it easier to read (Which is a very subjective optimization).

You could try and use the String Builder instead of concatonating strings, but without measuring it I'm not sure it would buy you any benefits.

Answer 2

Your best bet would be to use a StringBuilder object, so you simply need to append your HTML code. This will reduce the actual code length that you are reading.

I'm not sure where your storeSelected/areaSelected type values are coming from, but if they are coming from the DropDownLists themselves, you can check to see if an item has actually been selected in the DropDownList and process it that way. You can also use the SelectedValue property in the DropDownList to display the value. Your current code is simply going to return a ListItemObject.

Finally, it looks like your generated results are best going to be bound to an HTML literal control to properly render the results.

I realize there are a few assumptions in all of this, but if they are correct, your new code is going to look something like this:

If txt_panview3_ddlinput1.SelectedIndex > -1 Then
    tempCollector.Append("<br>" & "Department" & ": " & txt_panview3_ddinput1.SelectedValue & "<br>" & "Store" & ": " & txt_panview3_input2.Text & "<br>")
End If

If txt_panview0_ddinput1.SelectedIndex > -1 Then
    tempCollector.Append("<br>" & "Area Name" & ": " & txt_panview0_ddinput1.SelectedValue)
End If

If txt_panview1_ddinput1.SelectedIndex > -1 Then
    tempCollector.Append("<br>" & "Department " & ": " & txt_panview1_ddinput1.SelectedValue)
End If

If txt_panview2_ddinput1.SelectedIndex > -1 Then
    tempCollector.Append("<br>" & "Region " & ": " & txt_panview2_ddinput1.SelectedValue)
End If

If storeSelected() = 0 Then
    tempCollector.Append("<p><b>" & lbl_viewTitle2.Text & "</b>")
    tempCollector.Append("<br>" & lbl_view2_ManagersEmailAddress.Text & ": " & txt_view2_ManagersEmailAddress.Text)
End If

litResults.Text = tempCollector.ToString()

You're still looking at a pretty "brute force" process to generate your final text, but you've reduced the amount of tempCollector references and don't have a secondary function to trigger adding the proper value.

As a sidenote, if you needed the "displayed" value in the DropDownList, replace the .SelectedValue references to .SelectedItem.Text

Answer 3

You should explore using StringBuilder to replace all that string concantenation - it is very likely that it would be faster.

For most of the code you are repeating the same thing so you might consider replacing the repeated code with a subroutine which you call repeatedly. If might look like something like this [untested]:

void AddControlText(StringBuilder builder, string label, string value)
{
builder.Append("<br>");
builder.Append(label);
builder.Append(value);
}

EDIT:

Bit short on time so here is a start. You would use the above method and call it like this:

StringBuilder _Builder = new StringBuilder();

If (storeSelected = 1) Then
{
    AddControlText(_Builder, "Department: ",txt_panview3_ddinput1.SelectedItem.ToString);
    AddControlText(_Builder, "Store: ", txt_panview3_input2.Text);
}

EDIT(2): Woops just realised you are using VB - and the above is C# - hopefully you can translate it back to VB!