Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
The two examples do not do the same thing. The first example ignores the situations where a user does not exist or multiple users are found. The second example forces the caller to handle these situations or the program does not continue. –  Mike May 8 at 14:14
    
So you're basically trying to decide between the second snippet you posted and creating your own Exception sub-classes which you'd use in the same way? I'd say the difference between the two is probably relatively minor, it's not something to get too hung up on –  Ben Aaronson May 8 at 14:18
    
@BenAaronson agreed but it does seem to me that getting a count of records returned from the query and then getting the data returned from it later is inefficient. I'm basically just trying to get an idea of best practice –  Ortund May 8 at 14:19
5  
I wish there was anything in software engineering and design that has a best way to do it... I'd go work on that rather than thrashing about in real software development where I've always got to decide on tradeoffs between imperfect solutions –  Jimmy Hoffa May 8 at 14:28
    
Very well said, Jimmy Hoffa –  Ortund May 8 at 14:37

6 Answers 6

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.

share|improve this answer
    
I disagree, logging exceptions in production code can be extremely useful for debugging. Well, sort of disagree anyway- you mention this, but only say it's "possibly" useful, then seem to ignore it in your conclusion. –  Ben Aaronson May 8 at 14:46
    
@BenAaronson I disagree with your disagreement :D logging in production is in fact, very useful if done correctly. If it doesn't hurt performance is a huge step on reproducing bugs and even measuring another things such as response time, auditing, etc. –  mau May 8 at 15:09
    
@mau That sounds more like agreement with my disagreement –  Ben Aaronson May 8 at 15:13
    
So, if I intend to log the error and have the system email it to me to be fixed, that would be "useful"? –  Ortund May 8 at 16:51
    
Also, as far as the code currently stands, I just want to communicate the problem to the user... –  Ortund May 8 at 16:53

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.

share|improve this answer

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.

share|improve this answer

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
}
share|improve this answer

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

share|improve this answer
    
I suspect that what Ortund is asking about mostly boils down to what you've hidden behind analizeException. –  Ben Aaronson May 8 at 15:00
    
@BenAaronson actually analizeException is just a regex evaluation to check for database messages and translate to human readable text. All the meat is on the code I provided. –  mau May 8 at 15:05
    
Ah, okay. I can see how this would be useful, though it depends a bit on the architecture. For example, if a repository pattern is being used on top of EF's, that might be a more natural place to put this kind of exception "routing" code. –  Ben Aaronson May 8 at 15:15
    
Sure, it is an abstract solution possibly won't work with a different arquitecture. Just wanted to point out an idea which I reuse for many projects. I haven't used EF for long time, I'd rather use ServiceStack OrmLite. –  mau May 8 at 21:05

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.

share|improve this answer
1  
I don't think Exceptions are being used here to control application flow. They're essentially being used as preconditions. There's nothing wrong with that. Sensible checks like this can ensure that useful exceptions are thrown from the useful places when unexpected conditions arise, instead of problems propagating through the system and causing strange behaviour and/or exceptions to be thrown somewhere much harder to debug from. What you're arguing is only true if you assume there are no bugs in any code that interacts with the DAL- which is never a sensible starting point. –  Ben Aaronson May 9 at 0:43
    
Why not code them before calling update perhaps in the same location at the concurrency check. That way when update is called only exceptions are thrown. I can see your point though as that's a matter of style. –  suing May 13 at 21:07
    
I'm not sure what you mean by concurrency check in this context. But assuming that Update can be called from multiple places, having to do the check before you call it is likely to mean having to repeat yourself. –  Ben Aaronson May 13 at 23:34

protected by Community May 8 at 21:31

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.