views:

507

answers:

4

I have this sort of format

asp.net MVC View -> Service Layer -> Repository.

So the view calls the service layer which has business/validation logic in it which in turns calls the Repository.

Now my service layer method usually has a bool return type so that I can return true if the database query has gone through good. Or if it failed. Then a generic message is shown to the user.

I of course will log the error with elmah. However I am not sure how I should get to this point.

Like right now my Repository has void return types for update,create,delete.

So say if an update fails should I have a try/catch in my repository that throws the error, Then my service layer catches it and does elmah signaling and returns false?

Or should I have these repository methods return a "bool", try/catch the error in the repository and then return "true" or "false" to the service layer what in turn returns "true" or "false" to the view?

Exception handling still confuses me how handle the errors and when to throw and when to catch the error.

A: 

First of all there is no one way and there certainly isn't a perfect one so don't overthink it.

In general you want to use exceptions for exceptional cases (exceptions incur a performance overhead so overusing them especially in "loopy" situations can have a perf impact). So let's say the repository cannot connect to the database server for some reason. Then you would use an exception. But if the repository executes a search for some object by id and the object is not found then you would want to return null instead of throwing an exception saying that object with ID x doesn't exist.

Same thing for the validation logic. Since it's validating it is assumed that sometimes the input won't validate so in that case it would be good to return false from the validation service (or perhaps a more complex type including some additional information as to why it didn't validate). But if the validation logic includes checking if a username is taken or not and it can't do this for some reason then you would throw an exception.

So say if an update fails should I have a try/catch in my repository that throws the error, Then my service layer catches it and does elmah signalling and returns false?

Why would the update fail? Is there a perfectly fine reason for this happening that's part of the normal process? Then don't throw an exception if it happens because of a strange reason (let's say something removed the record being updated before it was updated) then an exception seams logical. There really is no way to recover from this situation.

olle
"search for some object by id and the object is not found then you would want to return null" ...and then hope your caller checks for null? If you expect the ID to be there (not user input), generally throw. If you're not sure, consider the Try-Parse pattern. See http://blogs.msdn.com/kcwalina/archive/2005/03/16/396787.aspx
TrueWill
It's up to the caller to understand the expected results from calling the function. If the caller ignores the fact the signature/documentation states the function can return nulls, then the caller is at fault not the function. Returning nulls is perfectly valid in this case IMOH.
BlackMael
Of course you could also argue that the caller may not expect exceptions to be throw, and it doesn't bother wrapping the call with a Try Catch.Either way you cannot really force the caller to do anything at all. So it is a moot point to use the actions of the caller as a deciding factor of returning nulls or throwing exceptions.
BlackMael
A: 

I like to think of exception handling this way: You define your method signature, as to what you are expecting to do. Now if you are not able to do that, then you must throw an exception. So if you are expecting something to fail based on the input data that you have, (ignoring the ambient state), then your method signature must indicate whether an operation has succeeded or failed. But if your method is not expecting to fail based on the input you have (again, ignoring all the other ambient state), then an exception is in order when the method fails.

Consider these two APIs:

int int.Parse(string integerValue); // In this case, the method will return int
                                    // or it will die! That means your data must be
                                    // valid for this method to function.

bool int.TryParse(string integerValue, out number); // In this case, we expect the data
                                                    // we passed in might not be fully
                                                    // valid, hence a boolean.
Charles Prakash Dasari
+3  A: 

The rule of thumb I always use is:

  • At low levels, throw when an operation cannot complete due to exceptional circumstances.
  • In middle layers, catch multiple exception types and rewrap in a single exception type.
  • Handle exceptions at the last responsible moment.
  • DOCUMENT!

Here's an example in pseudocode for a multi-layer ASP.NET MVC app (UI, Controller, Logic, Security, Repository):

  1. User clicks submit button.
  2. Controller action is executed and calls into the Logic (business) layer.
  3. Logic method calls into Security with the current User credentials
    • User is invalid
      • Security layer throws SecurityException
      • Logic layer catches, wraps in LogicException with a more generic error message
      • Controller catches LogicException, redirects to Error page.
    • User is valid and Security returns
  4. Logic layer calls into the Repository to complete action
    • Repository fails
      • Repository throws RepositoryException
      • Logic layer catches, wraps in LogicException with a more generic error message
      • Controller catches LogicException, redirects to Error page.
    • Repository succeeds
  5. Logic layer returns
  6. Controller redirects to the Success view.

Notice, the Logic layer only throws a single exception type -- LogicException. Any lower-level exceptions that bubble up are caught, wrapped in a new instance of LogicException, which is thrown. This gives us many advantages.

First, the stack trace is accessible. Second, callers only have to deal with a single exception type rather than multiple exceptions. Third, technical exception messages can be massaged for display to users while still retaining the original exception messages. Lastly, only the code responsible for handling user input can truly know what the user's intent was and determine what an appropriate response is when an operation fails. The Repository doesn't know if the UI should display the error page or request the user try again with different values. The controller knows this.


By the way, nothing says you can't do this:

try
{
  var result = DoSomethingOhMyWhatIsTheReturnType();
}
catch(LogicException e)
{
  if(e.InnerException is SqlException)
  {
    // handle sql exceptions
  }else if(e.InnerException is InvalidCastException)
  {
    // handle cast exceptions
  }
  // blah blah blah
}
Will
This technique presents the following problems:1) Any time the lower level assembly introduces more checks, or more exceptions, all the layers above it should change to wrap the exception properly - otherwise the wrapping just becomes a glorified catch-all and won't have much meaning2) Using different exception types for different errors increases the fidelity of error handling and if we just have only one exception, wherever you handle it, you will have to write your casing logic (if LogicException is because of something than handle otherwise let it go).There could be others too ...
Charles Prakash Dasari
1) Baw. You'd have to catch these new exceptions anyhow, so why cry when its at a lower level rather than at the UI? 2) If you do lots of stuff you could possibly be catching tens of exception types at the UI level. This is incredibly annoying and litters your code with logic at the UI that really shouldn't be there. Again, you are WRAPPING the original exception, so you can still CATCH a single type and examine InnerException for all your fidelity needs without lots of catches. 3) There could be other rebuts but I'm too lazy to list them.
Will
A: 

While returning an error (or success) code is often the better way, exceptions have one huge advantage over returning codes or silently suppressing errors: at least you can't just ignore them!

Don't abuse exceptions for simple flow control - that would be the silliest thing to do.

But if a function of yours really runs into an "exceptional" problem, then definitely throw an execption. The caller must then either handle it explicitly and thus know what's going on, or it'll bomb out on him.

Just returning an error code is dangerous since the caller might just not bother inspecting the code and could possibly still go on - even if in your app's logic, there's really something wrong and needs to be dealt with.

So: don't abuse exceptions, but if a real exception happens that requires the caller to do something about it, I would definitely recommend using that mechanism for signalling exceptional conditions.

As for handling exceptions: handle those that you can really deal with. E.g. if you try to save a file and get a security exception, show the user a dialog asking for some other location to save to (since he might not have permissions to save to where he wanted to).

However, exceptions you can't really deal with (what do you want to do about a "OutOfMemory exception", really?) should be left untouched - maybe a caller further up the call stack can handle those - or not. Marc

marc_s