views:

121

answers:

6

I find myself using exception in web development even for conditions that are not really errors, let alone exceptional ones - just logic decisions, validations...

in a web page, I often write code like so:

try
{
    int id;
    if(!int.TryParse(txtID.Text, out id))
     throw new Exception("ID must be an integer");

    if(IdAlreadyExists(id))
     throw new Exception("ID already exists in database");

    //and so on...
}
catch(Exception ex)
{
    SetErrorLine(ex.Message);
}

I was wondering if this is really the correct way of using exceptions and enforcing Business Logic in web development.

P.S.: I am using asp.net, and obviously I could use ASP.NET validators for some of these and also seperate UI from logic, but I'm trying to make a point on the general idea.

+6  A: 

You've just answered your own question, really.

Exceptions are for exceptional circumstances. Assuming the code you posted is from a page that takes "ID" as input from the user, then it's not exceptional for user input to be bad. Use the validation infrastructure for that, or do it manually, but don't use exceptions for that.

Also, don't get into the habit of displaying Exception.Message to users. It is meant for developers to see to know what went wrong and how to fix it.

John Saunders
+1  A: 

I wouldn't consider the first example to be an acceptable use of an Exception. A user could easily screw up entering that information, and user input should be validated anyways.

An exception should be 'exceptional'. Something that you don't expect to happen, shouldn't happen or really don't want to happen. Anything that can be validated or handled before needing to throw an exception should just be treated like an error.

Brandon
A: 

Different communities differ in their acceptable use of exceptions. The Python community is a little bit more liberal with their use of exceptions: http://wiki.sheep.art.pl/Ask%20for%20Forgiveness

I'm a fan of using them as long as it doesn't disrupt the logic of your application.

Dan Lorenc
@Dan: be aware that .NET exceptions were designed and optimized to be used for exceptional conditions. It's a mistake to use a framework in ways for which it was not designed.
John Saunders
A: 

Just wanted to add that for asp.net adding a global exception handler HttpModule to catch unhandled exceptions is a very clean way to train yourself out of try/catch/finally abuse.

annakata
A: 

Throw exceptions when the assumptions your methods take on are broken, or if a method simply cannot complete.

In your example, your IdAlreadyExists method might query a database. That method will assume that there is a database to connect to, you have enough memory to do what you need, and so on. If an assumption is broken, throw an exception.

I, for one, like to let lower level code throw exceptions, and higher level code catch exceptions. That's not a hard and fast rule, just a general guideline I like to follow. It's typically not necessary to throw exceptions to the same level, but I could imagine that it would make sense at times.

Stuart Branham
A: 
still missing the way it SHOULD be done

Set an error mesage and return.

Log error and dont display it to user.

protected void Button1_Click(object sender, EventArgs e)
{
    try
    {
     int id = 0;
     if(!int.TryParse(txtID.Text, out id))
     {
      SetErrorLine("Thats not a valid number dude! Need to go to school again");
      return;
     }

     if(IdAlreadyExists(id))
     {
      SetErrorLine("I already have this entry in store. Sorry. Call again!");
      return;
     }

     ...the rest of your code
    }
    catch(Exception ex)
    {
     _log.Error(ex);
     SetErrorLine("Something unexpected happened dear user ... ");
    }
}

Edit: your comment

try { 
 string error = ""; 
 int id = 10; 
 if(BO.DoSomething(out error, id, otherParams)) 
 { 
  SetErrorLine(error); 
  return; 
 } 
} 
catch(Exception ex)
{ 
//log 
}

My guess:

try { 
 string meaningfullErrorMessage= ""; 
 int id = 10; 
 if(!BO.DoSomething(out meaningfullErrorMessage, id, otherParams)) 
 { 
  SetErrorLine(meaningfullErrorMessage); 
  return; 
 } 
} 
catch(Exception ex)
{ 
//log 
}
Malcolm Frexner
So for every business object this would be the recommended syntax:try{ string error = ""; int id = 10; if(BO.DoSomething(out error, id, otherParams)) { SetErrorLine(error); return; }}catch(Exception ex){ //log}
See the edited code. It depends on the errormessage you return. If it is a meaningfull message this would be correct. If it you just assign ex.Message to out error than it is not ok. Your BO should throw again the execeptions it catches. Otherwise you loose the stacktrace.
Malcolm Frexner