CODEWITHSUNDEEP
c#try-catch
GLP
GLP

Reputation: 3675

What are best practices for handling exceptions in C#?

I have following code in my web page:

btnTest_Click(object sender, EventArgs e)
{
    ...
    bool ret=myFunc(...);
    if (ret)
    {...}
    else
    {
        lblStatus.Text="Some Text";
        lblStatus.Visible=true;
    }
}

private bool myFunc(...)
{
    bool ret=false;
    try
    {
        ...
        ret=true;
    }
    catch (Exception ex)
    {
        lblStatus.Text="Other Text";
        lblStatus.Visible=true;
    }
    return ret;
}

If an exception occurs in myFunc, the lblStatus always shows "Some Text" not "Other Text". That means the catch block in myFunc doesn't really mean anything. I wonder how to fix this code to handle the exception better?

update: maybe my example is not very good. But I main purpose is to ask best practices for exceptions handling between calling and being called functions.

Upvotes: 1

Views: 274

Answers (7)

Luis Filipe
Luis Filipe

Reputation: 8708

Your catch clause is doing a lot. It catches every exception and "forgets it" suppressing it to the rest of the call stack. This can be perfectly fine but i'll try to explain your options:

You usually have 3 options:

  1. Do not care about exceptions and let code above you handle it
  2. Care to log the exception and let it propagate
  3. The exception has its meaning in a given context and should not be propagated (this is your scenario)

I use all of them.

Option 1

You can just implement your function and if an exception occurs then it means some fault occurred and you just want your application to fail (at least to a certain level)

Option 2

Some exception occurs and you'll want to do one of two (or even both)

  • log the error
  • change the exception to another one more meaningful to the caller

Option 3 The exception is expected and you know how to completely react to it. For instance, in your case, i tend to believe you do not care about the type of exception but want a "good default" by setting some controls to a given text.

conclusion

There are no silver bullets. Use the best option for each scenario. Nevertheless catching and "suppressing" catch(Exception ex) is rare and if seen often it usually means bad programming.

Upvotes: 1

Corgalore
Corgalore

Reputation: 2566

I usually setup an error handling system. Here's a simple way, but this can be wrapped up into a base class. I can show you that if you need.

List<string> _errors;

void init()
{
 _errors = new List<string>();
}

protected void Page_Load(object sender, EventArgs e)
{
  init();
}

btnTest_Click(object sender, EventArgs e)
{
    ...
    var result = myFunc(...);
    if (result)
    {...}
    else
    {
        if (_errors.Count > 0)
        {
          var sb = new StringBuilder("<ul>");
          foreach (string err in _errors)
          {
            sb.AppendLine(string.Format("<li>{0}</li>", err));
          }
          sb.AppendLine("</ul>");
          lblStatus.Text=sb.ToString();//Make this a Literal
        }
    }
}

private bool myFunc(...)
{
    var result = true;
    try
    {
        ...
        ...        
    }
    catch (Exception ex)
    {
        result = false;
        _errors.Add(ex.Message);
    }
    return result;
}

Upvotes: 0

Peter Bolton
Peter Bolton

Reputation: 1

If you want to be informed of encountering a specific type of error inside one of your functions, I'd recommend inheriting Exception and creating your own exception class. I'd put a try-catch block inside your btnTest_Click() handler, and then I'd look to catch your custom exception class. That way, you won't lose the opportunity to detect any errors happening inside your myFunc() function.

Upvotes: 0

Chris
Chris

Reputation: 27384

This is a complex question so I will try to break it down

  1. In terms of functions I would try to stick to the Single Responsibility Principal. It should do one, well defined thing.
  2. Exceptions should be that, exceptional. It is then preferable to try not to incur exceptions but obviously to deal with them as and when. For example it is better to test a variable as being null before attempting to use it (which would throw an exception). Exceptions can be slow (especially if a lot are thrown)
  3. I would say that the question of WHERE you handle the exception is down to whose responsibility the exception is. If myFunc were to access a remote server and return a status of true or false you'd expect it to handle its own IO exception. It would probably not handle (or rethrow) any parameter problems. This relates to point 1. It is the functions responsibility deal with the connection process, not to provide the correct parameters. Hiding certain exceptions can cause problems if other people (or a forgetful you) tries to use the code at a later date. For example in this myFunc which makes a connection, should you hide parameter exceptions you may not realise you have passed in bad parameters

Upvotes: 0

Guffa
Guffa

Reputation: 700152

The exception handling is just fine. The problem with your code is that you are putting the "Some Text" string in the label if the return value is false, and that is when there was an exception, so it will replace the message from the catch block.

Switch the cases:

if (ret) {
  // it went well, so set the text
  lblStatus.Text="Some Text";
  lblStatus.Visible=true;
} else {
  // an exception occured, so keep the text set by the catch block
}

Upvotes: 0

Nicholas Carey
Nicholas Carey

Reputation: 74177

Why is your called function setting the label text on exception and the caller setting it on success?

That's something of a mixed metaphor. Let one party be responsible for UI (separation of concerns) while the other is responsible for doing work. If you want your called function to be fault tolerant try something like this:

private bool myFunc(...)
{
  bool ret ;
  try
  {
    ...
    ret=true;
  }
  catch
  {
    ret = false ;
  }
  return ret;
}

Then your caller can do something like:

bool success = myFunc(...) ;
lblStatus.Text = success ? "Some Text" : "Other Text" ;
lblStatus.Visible = success ;

if ( success )
{
  // do something useful
}

Upvotes: 4

dcastro
dcastro

Reputation: 68640

It displays "Some Text" because, when an exception occurs in myFunc, it returns false. Then you go into the else block of the btnTest_Click method, where you set lblStatus.Text to "Some Text" again.

So, basically, you're setting the label's text to "Other text" and then to "Some Text".

Upvotes: 1

Related Questions