3

I have a large project in C# (.NET 2.0) which contains very large chunks of code generated by SubSonic. Is a try-catch like this causing a horrible performance hit?

    for (int x = 0; x < identifiers.Count; x++)
            {decimal target = 0;
                try
                {
                    target = Convert.ToDecimal(assets[x + identifiers.Count * 2]); // target %
                }
                catch { targetEmpty = true;  }}

What is happening is if the given field that is being passed in is not something that can be converted to a decimal it sets a flag which is then used further along in the record to determine something else.

The problem is that the application is literally throwing 10s of thousands of exceptions as I am parsing through 30k records. The process as a whole takes almost 10 minutes for everything and my overall task is to improve that time some and this seemed like easy hanging fruit if its a bad design idea.

Any thoughts would be helpful (be kind, its been a miserable day)

thanks, Chris

Christopher Klein
  • 2,773
  • 4
  • 39
  • 61
  • 1
    Try to read the following sentence and then replace each `Ouch!` instance with an exception and extrapolate the effect on your code: Yes, `Ouch!` there `Ouch!` could `Ouch!` be `Ouch!` a `Ouch!` small `Ouch!` perf `Ouch!` hit `Ouch!` from `Ouch!` all `Ouch!` these `Ouch!` exceptions. – Franci Penov Apr 22 '10 at 21:24
  • I only started looking at this code recently... actually it looks like there are 3 similar tests testing if the 3 fields actually ARE decimals or if they are blank/null/non-decimal. Then if all 3 flag fields are true it doesn't try to add the record. Ugh... yeah, that is pretty sad. – Christopher Klein Apr 22 '10 at 21:38

4 Answers4

6

Using exceptions for control flow is generally a bad practice (exactly because of the poor efficiency that you're observing). What type of data do you need to convert to decimal? Could you use TryParse method or some other method that doesn't throw exception if the input is not in the expected format?

Decimal.TryParse method should do the trick if you're parsing strings as it reports failure by returnning false:

decimal d;
if (Decimal.TryParse(str, out d)) 
  // Ok, use decimal 'd'
else 
  // Failed - do something else
Tomas Petricek
  • 240,744
  • 19
  • 378
  • 553
  • many thanks, I replaced the 3 places where that particular horrid code was with the above and now the whole thing finished in LESS than 30 seconds! – Christopher Klein Apr 22 '10 at 22:43
5

That is a truly horrible looking piece of code. In general exceptions should only be used to capture unexpected results. The fact that you are getting lots of exceptions means that this is a common condition that should become part of the intrinsic logic.

decimal.TryParse would be a far better option here and I would suggest caching the value of identifiers.Count * 2 as well (not sure if compiler will optimise that)

James Westgate
  • 11,306
  • 8
  • 61
  • 68
3

There's a TryParse for Decimal as well. It avoids the exception and uses a bool instead to signal success/failure.

Brian Rasmussen
  • 114,645
  • 34
  • 221
  • 317
0

It's scary looking code, and you're getting good suggestions on how to fix it.

If you want to know if those exceptions are costing you a big percentage of time, there's a simple way to find out.

Just pause it about 10 times, and each time examine the call stack. If the exceptions are costing some percentage of time like, say, 50%, then you will see it in the process of throwing or catching them on roughly that percent of pauses.

Mike Dunlavey
  • 40,059
  • 14
  • 91
  • 135