1

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.

4
  • Aside from the best practices part, which I don't have enough experience to answer, myFunc would be much simpler if you set bool ret = true; to begin with and then simply set it to false in the catch block. Commented Nov 11, 2013 at 22:54
  • Have you stepped through the code? Are you sure myFunc() is returning false? Commented Nov 11, 2013 at 22:55
  • 1
    Well the catch block does mean something. Without it, lblStatus would never change because you'd have an unhandled exception. Commented Nov 11, 2013 at 22:59
  • I'm not entirely sure this has much to do with exception handling as it does with not having a spec for the page. Draw out the flow chart for how the page should function and code towards that. Commented Nov 11, 2013 at 23:04

7 Answers 7

4

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
}
Sign up to request clarification or add additional context in comments.

2 Comments

This is what I was getting at in my comment, but wouldn't it be better to simply initialise ret as being true? The try block will always be executed.
@JohnH: if you don't initialize the return value, you'll get an error about an uninitialized value if you try to return out of the method body. Explicity setting it at the completion of work means that when somebody down the line decides to just return out of the method, they'll be reminded to fulfill the method's contract (by setting an appropriate status before returning.
1

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.

Comments

1

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".

3 Comments

Ha, I read OP wrong and thought he said it was always displaying "Other text"
Then what should I do to handle the exception better?
Nicholas Carey already explained how you should be doing this.
0

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
}

Comments

0

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

Comments

0

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.

Comments

0

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;
}

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.