So I’m a little concerned about my error handling… Currently my execution path looks something like this:
Users.aspx -> App_Code/User.cs -> Data Layer/User.cs
So now when I try to update a user record, I put my Try/Catch block in the event handler and ensure that only the App_Code class interacts with the data layer. Exceptions that happen on the data layer, to my understanding, should bubble up to the event handler below.
In the data layer, I started off with this:
public void Update()
{
var product = (from p in db.products
where p.productid == id
select p).FirstOrDefault();
if (product != null)
{
// update the thing
}
}
More info on reddit.
After chatting with a friend, he recommended something like this:
public void Update()
{
int count = db.users.Count(u => u.userid == id);
if (count == 0) // no user found
{
throw new ValidationException(String.Format("User not found for id {0}.", id));
}
if (count > 1) // multiple users
{
throw new ValidationException(String.Format("Multiple users found for id {0}.", id));
}
var user = db.users.FirstOrDefault(u => u.userid == id);
// update the user record
}
Then I went onto IRC where they suggested I create my own Exceptions.
I can see the pros here, but it seems a bit unnecessary when my friend’s option will work just fine.
Basically I’m just really confused as to how I should handle this… Obviously my initial option is insufficient, but it seems like creating my own exceptions might be complicating things too much.
So what should I do here?
7
The point of writing custom exceptions is that you intend to do something useful with them.
Is showing them to the user “useful”?
Probably not. They look scary.
Is logging them to a file for later examination “useful”?
Possibly, especially if the application is about to keel over and die because you’re doing this in a “global” exception handler (which is about all they’re good for).
Is catching a particular Type of exception and handling it (i.e. writing code to deal with the problem as it happens and to correct that problem, preferably without the user knowing anything about it) “useful”?
Oh yes!
Why use custom Exception Types? Because that’s how most languages expect to identify exceptions. Look at “catch” clauses – they look for specific Types of Exception. Sorry to say it but I would vehemently disagree with your friend’s recommendation – throwing the same [class of] ValidationException [objects] all over the place but relying on the Text property to explain what’s going on. That’s only useful in one scenario – where you show Exceptions directly to the user and that’s a pretty poor practice, in my book anyway.
5
In this particular situation, a better option may be to use LINQ’s Single method, as so:
public void Update()
{
var user = db.users.Single(u => u.userid == id);
// update the user record
}
Single already does exactly what you want: it throws an Exception if there are 0 or more than 1 results matching the predicate.
At this point you can decide whether you’re happy for the exception thrown by Single to bubble up, or whether you want to wrap it in one with a more useful message, or a specific type. The general pattern for this would be:
public void Update()
{
try
{
var user = db.users.Single(u => u.userid == id);
}
catch(SomeExceptionType ex)
{
throw new SomeOtherExceptionType("Useful message here", ex);
}
// update the user record
}
(Notice passing ex to the SomeOtherExceptionType constructor here. This allows the information about the original exception to be preserved, as is usually good general practice)
As I said in the comment, the choice of how exactly you do this is probably not overly important. My advice would be to start with the simplest option- in this case allowing Single to throw its own exception- and refactor as needed.
If you find yourself with the need to display or log a more specific exception message, either wrap the exception in this method, or higher up the call chain as appropriate. The principle to where you do this should be avoiding leaking between your abstraction levels. Consider the following:
public void HighLevelMethod()
{
try
{
DataAccessClass.Update();
}
catch(Exception ex)
{
throw new SomeKindOfException("What should I say here?", ex);
}
}
The message here should be at an appropriate abstraction level for HighLevelMethod. Something along the lines of “The update failed” (though ideally something a bit more useful!). The exact reason for the update to fail is hidden within Update, so for the message to be “The update failed because no user was found” would be causing a leak of implementation details between the abstraction layers. If you wanted to specify that level of detail, that message would need to go in an exception thrown by Update itself.
Likewise, only refactor to throwing your own Exception sub-class if that provides some useful information that you find yourself needing higher up the call chain. Will the code calling Update() have a different way of handling a UserNotFoundException than it would a ValidationException? If not, then don’t bother with your own Exception type.
If you’re writing a library to be used by some other, external, code, you have to be a little more pro-active in working out when providing special messages or custom Exception types will be useful, rather than just waiting until there’s a need. But the same general principles will apply.
What makes the most sense to you? Does a ValidationException accurately describe the problem? If so, there’s nothing wrong with using that as is and passing a good, valid message along with it. If you want to make a more specific error message, create an Exception class that inherits ValidationException, say, “UserNotFoundException” and “ManyUsersFoundException”. These are probably poor names and not what you want, but you get the idea.
Also…
var user = db.users.FirstOrDefault(u => u.userid == id);
Your code here is a bit redundant to use “FirstOrDefault”. You already know, by this point, you have a record. So you can safely use:
var user = db.users.First(u => u.userid == id);
And get away with it with no consequences.
Other than that, I’d say your modifications based on your chat with your friend seem more than enough to me. But that’s a personal opinion.
Basically it depends on how you want to handle your catch statements. The advantage of having your own custom exceptions is you can catch them separately and execute separate pieces of code, see below. However if you just plan on handling all types of exceptions the same like dumping the message to a log file or message box, using a generic exception is fine.
catch (UserNotFoundException ex)
{
//handle code for user's not found
}
catch (MultipleUsersFoundException ex)
{
//handle code for multiple users
}
catch (Exception ex)
{
//handle other badness
}
I don’t use Entity Framework but I think the same rule applies. What I usually do is wrap all the database calls within a lambda and reuse that block of code for every operation. Outside the lambda I use a try/catch block, so any exception gets routed to the same path, therefore I don’t need to explicity treat each error on their own. If the result happens to throw an exception, I rollback otherwise I commit.
If there is a special message that I need to send information to the client, usually I throw an inner exception into the lambda code so it gets bubbled outside the lambda and the client recieves a non-technical message.
In my opinion all the exceptions should be logged somehow, again I do this at a higher level, outside the lambda as a warning (e.g. a referential integrity error is not necessary a bug, but a missing value).
As for the Insert / Update logic you’re showing it depends whether you are using one method for create and another method for update. If this is the case, then you should raise an exception if the id is not found on the update method. However, I usually like to “upsert” rather than having two methods so like you do, if the id is not found on the database, I assume this is an insert.
Maybe looking into this example it will be clear what I mean: Database Wrapper
4
TL;DR;
What I would say is that those are not exceptions to you because you should have checked them before calling update. By the time you got to that point of update the only thing that should fail is a network error, db lock, or db not available; behavior out of your control. If you check everything you are in control of then your application flow will be yours to control and not based on exceptions. Exceptions are expensive so you should avoid using them to control application flow.
3