views:

160

answers:

3

Hi guys!

Let's assume we have the following method in the business layer. What's the best practice to tell the UI layer that something went wrong and give also the error message? Should the method return an empty String when it was OK, otherwise the error message, or should it throw another exception in the catch code wrapping the caught exception? If we choose the second variant then the UI should have another try,catch which is too much try,catch maybe. Here is a pseudocode for the first variant.

public String updateSomething()
{
   try
   {
      //Begin transaction here
      dataLayer.do1();
      dataLayer.do2();
      dataLayer.doN();
      //Commit transaction code here
   }
   catch(Exception exc)
   {
      //Rollback transaction code here
      return exc.message;
   }

   return "";
 }

Is this a good practice or should I throw another exception in the catch(then the method will be void)?

+1  A: 

It's a little unusual to return a String like this (but there's no real reason not too). More usual methods would be:

  • return a boolean value, and have some method of setting the error message, either by logging it, setting some global "last error" value, or having a pointer to an error construct passed in to your method which you update;

  • have a void method which throws an exception on failure, and handle it in the calling code (as you suggest)

I have see both of the above used extensively. It's hard to say which is "best". Try to be consistent with the idioms and conventions of the language you are working in and/or the existing code set/libraries you are working with if any.

invariant
+1, I love the first way. I personally hate the clutter of try-catch. Isn't it cool to write "if (updateSomething()) { }" instead of "try { updateSomething(); } catch (Exception) { }"?
Petar Minchev
I have a fondness for the straightforwardness of the "if" technique too. But there are benefits to the exceptions technique, like when you have to call a lot of methods in sequence, you can write: `try { something(); somethingElse(); anotherThing(); blah(); } catch(...) { } finally { alwaysRunThis(); }` instead of checking every if statement along the way. Horses for courses...
invariant
+4  A: 

The best way is wrap exception in some more general type and rethrow it. So updateSomething() must declare that it can throw some sort of Exception (for example: UpdateFailedException) and in catch block you should wrap exception.

public String updateSomething() {
  try {
    [...]
  } catch ( SQLException e ) {
    // rollback;
    throw new UpdateFailedException(e);
  }
}

But catching abstract Exception type is not a good idea. You should wrap only those things which semantic you know. For example: SQLException, DataAccessException (Spring DAO) etc.

If you wrap Exception you easily could wrap InterruptedException of NullPointerException. And this can broke your application.

dotsid
You are absolutely right. I just have the doubt, that I will end up wrapping with try,catch everything in the UI, that calls the Business Layer. Is this a problem?
Petar Minchev
I think so. There are some situations where you couldn't proceed and must stop application. There is no much sense in catching OutOfMemoryError or something like this. So, the main point is to separate errors which are restorable from errors which are not.
dotsid
+2  A: 

I like to return a standard contract to my UI layer from my business layer.

It looks like this:

public class ServiceOperationResult<T>
{

    public bool Successful
    {
        get; 
        set;
    }

    public ServiceErrorType ErrorType
    {
        get;
        set;
    }

    public string ErrorMessage
    {
        get;
        set;
    }

    public T ReturnData
    {
        get;
        set;
    }
}

I use generics so that every service can define what it sends back, and the standard error flags tell the client app what type of error occurred (these are a meta-type, like "Internal error", "External party error", "Business rule validation error") and the app can then react in a standard fashion to these error types.

For instance, business errors are displayed in a red error label, while internal errors get redirected to an error page (in a web app) or close the form (in a windows app)

My pet hate is seeing a red label on a web site (where I expect to see validation errors) and seeing something like "The database server refused your connection" This is the risk that you run by only using a string to return error data.

Joon
+1, very interesting idea.
Petar Minchev