CODEWITHSUNDEEP
c#optimization
BeefTurkey
BeefTurkey

Reputation: 910

How to optimize this code?

Surely there has to be many ways to optimize the following code, where I basically have to make sure that a lot of textboxes aren't empty and then read their values:

if (foo1.Text.Length != 0 & bar1.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo1.Text + " / " + bar1.Text;
}

if (foo2.Text.Length != 0 & bar2.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo2.Text + " / " + bar2.Text;
}

if (foo3.Text.Length != 0 & bar3.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo3.Text + " / " + bar3.Text;
}

if (foo4.Text.Length != 0 & bar4.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo4.Text + " / " + bar4.Text;
}

if (foo5.Text.Length != 0 & bar5.Text.Length != 0) 
{
    output.Text += myStrings[i] + " / " + foo5.Text + " / " + bar5.Text;
}

if (foo6.Text.Length != 0 & bar6.Text.Length != 0)
    output.Text += myStrings[i] + " / " + foo6.Text + " / " + bar6.Text;

if (foo7.Text.Length != 0 & bar7.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo7.Text + " / " + bar7.Text;
}

if (foo8.Text.Length != 0 & bar8.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo8.Text + " / " + bar8.Text;
}

if (foo9.Text.Length != 0 & bar9.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo9.Text + " / " + bar9.Text;
}

if (foo10.Text.Length != 0 & bar10.Text.Length != 0)
{
    output.Text += myStrings[i] + " / " + foo10.Text + " / " + bar10.Text;
}

Upvotes: 3

Views: 453

Answers (7)

Greg D
Greg D

Reputation: 44096

There are a lot of ways to refactor this. The method you choose will depend on your particular scenario and needs.

  1. Make a function that takes a foo and a bar as a parameter and returns the string, then aggregate that string in a string builder
  2. Put the foos and bars into collections and loop over those collections. In this scenario, arrays would be useful and provide a way to mutually index the arrays.
  3. Take item 2 one step further and create a new class FooBar that holds a foo and a bar together. This way you can create a collection of FooBars and you don't have an implicit association between them anymore, now it's explicit and codified.
  4. Take item 3 one step further and recognize that you're aggregating a string. If you're in a recent version of c#, take advantage of your Map/Reduce in LINQ (.Select().Aggregate()) to transform your FooBars into their corresponding strings, and then to aggregate the strings into output.

This is all just the stuff off the top of my head. If you work at it more, I'm sure you can do even better. :)

(If this is homework, please add a homework tag.)

EDIT:

I can't help but wonder about the design of the UI in general based upon your comment in another post stating that it takes "many seconds" to concatenate the strings together. 10 strings is a pretty trivial amount of time on its own, but this suggests that your outer loop (that's generating i) is fairly long running.

If you're in a position where you have the freedom to make such decisions, are you certain that your UI is actually good for the task at hand? A large collection of pairs of text boxes is a difficult user interface in general. Perhaps a ListView would be more appropriate. It would implicitly contain a collection, so you wouldn't have to deal with this "ten text boxes" silliness and will be an easier UI for your users to follow in general.

Upvotes: 2

Learning
Learning

Reputation: 8185

 foreach (Control ctrl in Page.Controls) {
      if (ctrl is TextBox) {
          if (ctrl.Text.Length != 0) {
              output.Text += myStrings[i] + "/" + ctrl.Text;
           }
      }
 }

Untested , but should work. With this your textboxes could be named anything.

Upvotes: 1

Vinay
Vinay

Reputation: 4793

Write a function which accepts foo & bar types. Pass all foo1 & bar1 to foo10 and bar10 to the function to get the values. You can also create array of foo and bar and you can loop and call the method to get the string.

Upvotes: 1

teedyay
teedyay

Reputation: 23531

Would it be feasible to make a WebControl that has textboxes called foo and bar, and that has a function like:

if (foo.Text.Length != 0 & bar.Text.Length != 0)
    return myStrings[i] + " / " + foo.Text + " / " + bar.Text;
else
    return string.Empty;

Put ten of those on your page, then use:

output.Text = myControl1.FooBar + myControl2.FooBar + myControl3.FooBar + ...

(Still a bit messy, but not quite so repetitive.)

Upvotes: 0

rix0rrr
rix0rrr

Reputation: 10276

I would put the repeated elements in arrays and then loop over them. 

TextBox[] foos = new TextBox[] { foo1, foo2, foo3, /* etc */ };
TextBox[] bars = new TextBox[] { bar1, bar2, bar3, /* etc */ };

for (int i = 0; i <= 10; i++)
    if (foos[i].Text.Length != 0 && bars[i].Text.Length != 0)
        output.Text += myStrings[i] + "/" + foos[i].Text + bars[i].Text;

Of course, if the elements are really named sequentially you can fill the arrays by looking up the controls from the form's Controls collection, with the name "foo" + number.ToString().

Upvotes: 9

Sol
Sol

Reputation: 2185

Could you make foo1-foo10 into an array, foo[10], and the same for bar? That would allow you to express this as a simple loop.

Upvotes: 0

Gerrie Schenck
Gerrie Schenck

Reputation: 22378

I would just loop over the Controls collection on which these TextBoxes reside, then filter on TextBox only and do you checks and concat.

I also strongly advice to use a StringBuilder instead of +=.

Upvotes: 5

Related Questions