Extracting Power and Energy out of a slow regex

Question

My WinForms application is a bit slow, I'd like to make it faster. It extracts only the power and energy values out of a string in textBox3, and displays the extracted values in textBox2 and textBox4.

 textBox3.AppendText(datarecv); // display data on text box
 temp = textBox3.Text;
 //Regular expression to only extract values of power and energy and is displayed on the meter 
 Regex regex = new Regex(@"Power consumption:\s=\s\[\s(?<PowerConsumption>\d*.\d*)\s\]");
 Regex regex1 = new Regex(@"ENERGY CONSUMED:\s=\s\[\s(?<EnergyConsumed>\d*.\d*)\s\]");
 MatchCollection matches = regex.Matches(temp);
 MatchCollection matches1 = regex1.Matches(temp);
 foreach (Match match17 in matches1)
 {
 aaa = (match17.Groups["EnergyConsumed"].Value);
 textBox4.Text = aaa;
 float bb3 = Convert.ToSingle(aaa);
 int bb12 = (int)bb3;
 // trackBar2.Value = bb12;
 try
 {
 trackBar2.Value = bb12;
 circularGauge1.Value = trackBar2.Value;
 }
 catch
 {
 }
 //R.E for energy
 }
 foreach (Match match in matches)
 {
 aaa = (match.Groups["PowerConsumption"].Value);
 textBox2.Text = aaa;
 float bb = Convert.ToSingle(aaa);
 int bb1 = (int)bb;
 // int result = 0;
 //if (int.TryParse(bb, out result))
 // trackBar1.Value = bb1;
 try
 {
 trackBar1.Value = bb1;
 circularGauge12.Value = trackBar1.Value;
 // R.E for Power
 }
 catch
 {
 }
 }

• Mathieu Guindon: How much text is there in textBox3? How many iterations for each loop?
• hjpotter92: Use \d+\.?\d* to match float numbers.

Answer 1

Exceptions aren't free...

 try
 {
 trackBar2.Value = bb12;
 circularGauge1.Value = trackBar2.Value;
 }
 catch
 {
 }

I'm not sure what exception you're swallowing here, but catching and swallowing exceptions in a loop doesn't sound very efficient. I suspect you wrapped these assignments in try/catch scopes because they were throwing an exception - the problem with it is that, well, it costs. Depending on the number of iterations and actual exceptions thrown and swallowed, this could very well be a nice little bottleneck.

Make if blocks that check for whatever the failure conditions are, and avoid throwing altogether.

---

Scoping

 aaa = (match17.Groups["EnergyConsumed"].Value);

How come aaa isn't declared within the scope you pasted here? It's used inside the foreach scopes, it should be scoped inside each foreach loop, with each loop having its own declaration.

Declare variables closer to their usage, you'll spare yourself lots of headaches, ...and bugs.

---

Comments

textBox3.AppendText(datarecv); // display data on text box

That isn't a useful comment, remove it. Same with these:

//R.E for energy

// R.E for Power

That "R.E." is really distracting. What does it mean? Do I need to know what it stands for? Oh wait, is that for "Regular Expression"? Then why is it at the bottom of the loop? ...so much time wasted, that could have been used reading, understanding and simplifying code.

---

You're doing too much.

You haven't inluded the entire method's scope, and already it's too much. That code is doing way too many things and has too many reasons to fail. You need to break down the problem into smaller, bite-size parts; the only way to eat an elephant is one bite at a time.

---

Name things.

Variables like temp, aaa and bb and then bb1 and bb12, matches and then matches1, regex andd then regex1, are all indicating the same thing: too much is going on, and too few things have a meaningful name.

Bad names make code harder to read and follow, and require more brain juice to process. In other words:

Be lazy, make the effort to name things.

---

I think there should be at least two methods here, one for each group of controls you want to update. Then, seeing how the two methods are pretty much identical except for a few things, you would parameterize things and refactor them into one. But before I can recommend anything about it, I'd need to have more context.

I'd recommend you name things (renaming is as easy as Ctrl+R,R in Visual Studio) and avoid the exception-throwing/swallowing, and ask yourself why you need two loops and how you could write your code so as to only iterate the data once... then post a new follow-up question (linking to this one) that includes at least an entire method, perhaps even the entire form's code-behind.