|
That code example made me shudder.
Separation of concerns; Is the concern database access or User notification, because it sure as hell shouldnt be both.
|
|
|
|
|
Well, as far as I'm concerned both would be nice. Sure, the connection to the database has priority over user notification, however when something stuffs up, I generally let the user know. In that particular case I would do something like :
private void DoSomething()
{
SqlConnection connection = null;
try
{
connection = new SqlConnection();
connection.Open();
}
catch(SqlException ex)
{
MessageBox.Show(ex.ToString();
}
finally
{
if (connection.ConnectionState == ConnectionState.Open)
connection.Close();
}
}
Then you actually have both of two worlds. However, that puts us right back to the essence of this discussion. Having a catch clause in a method is not something to be ashamed of, though I get the feeling that many developers think that way.
|
|
|
|
|
ahhh, no no no.
If your UI code is mixed with your database access code; you're doing it wrong
If you show Exception messages unsanitized to your users; you're doing it wrong
|
|
|
|
|
Totally agreed on the first point. But regarding the second, he did mention that he might put something else there.
I tend to have something which shows them the exception trace in the critical exception handler, because if there is a serious bug, you (the developer) want that information to fix it. In the case of a database connection failure, though, definitely not, because most of those reasons are not because your software is broken.
|
|
|
|
|
BobJanova wrote: you (the developer) want that information to fix it
And therein is the point. I want it, but I dont EVER want a user to see it. This is what the Windows Event Log (or, perhaps another type of log) is for.
You never have a reason to show a user an unsanitized exception message (caveat: they're programmers and can actually act on the information). There is the potential to give away sensitive information if you do so.
|
|
|
|
|
J4amieC wrote: This is what the Windows Event Log (or, perhaps another type of log) is for.
Or to put it another way - that's what log4net is for.
|
|
|
|
|
I totally agree with you, and I would never show an exception to a user. Hence the
, but I still think that you should at least say something when anything important messes up, like .. euhh .. a database connection that fails ?
I don't think it's wise to redirect general user messages that could be caused by an exception to the windows logs. Not very user friendly.
|
|
|
|
|
J4amieC wrote: If your UI code is mixed with your database access code; you're doing it
wrong If you show Exception messages unsanitized to your users;
you're doing it wrong
Ahhh, no. It depends upon the scope of the problem you're trying to solve. If this is a 1,000 line utility app that you're the only user for, this might be perfectly appropriate. If it's a 200,000 line client for a LOB app, then you might have issues.
Software Zen: delete this;
|
|
|
|
|
If you're in a helper layer such as a DAL then you don't want to add any code that will interact with the user, however you may want to do clean up before the exception is thrown up the chain to a level where the exception can be dealt with by the user. Equally what's the difference between the original example and the following?
Private Sub ExceptionMethod()
Try
DoExceptionCode()
Finally
DoCleanup()
End Try
End Sub
Private Sub ExceptionHandlerCode()
Try
ExceptionMethod()
Catch ex As Exception
ExceptionHandlerHere()
End Try
End Sub
|
|
|
|
|
Arrgggh - my eyes. The horror. Case insensitive code in our lovely curly bracketed case sensitive world.
|
|
|
|
|
Sorry, I'll use Smalltalk next time.
More seriously I had a vb editor open so it was just quicker to type it in there with the auto complete than to start up a new c# editor for the example (formatting purposes)
|
|
|
|
|
DragonLord66 wrote: I had a vb editor open
In the name of all that's holy man, why?
|
|
|
|
|
We have a very large code base of VB code that was written simply because it's easier to get a working prototype in vb (pre 2010 and c# runtime code editing), and the prototypes turned into production code...
|
|
|
|
|
Oh come on, its just another way of saying (explaining) the same thing.
|
|
|
|
|
Suppose the line
connection = new SqlConnection();
fails and connection stays null.
Won't you receive a very ugly "unhandled exception" message? (haven't tried it).
V.
|
|
|
|
|
You will, but the exception will bubble up (actually, in this case you won't get an exception. The zero parameter constructor doesn't do anything that can cause an exception). I deliberately didn't put exception handling in here to avoid clouding the issue.
|
|
|
|
|
that is perfectly legal. The outer try-catch will catch whatever gets thrown outside the inner try block, e.g. an exception occurring in the inner finally block.
Luc Pattyn [Forum Guidelines] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, improve readability, and make me actually look at the code.
|
|
|
|
|
this will hide the inner exception of the inner try?
Help people,so poeple can help you.
|
|
|
|
|
Well, if you don't use an inner catch, every exception in both the inner and outer try block, will be caught by the outer catch block.
|
|
|
|
|
the finally block is always executed, and any exception that gets thrown will be caught by the first surrounding and matching catch block. So the finally block will execute first (unless the catch belongs to the same try as the finally).
Luc Pattyn [Forum Guidelines] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, improve readability, and make me actually look at the code.
|
|
|
|
|
A catch block containing nothing but a rethrow is by definition pointless. That is,
try {
doStuff();
} catch {
throw;
}
... is equivalent to simply calling doStuff(). So, if you haven't simplified out some code from the catch block, the outer try is pointless.
However, in the general case, this type of construct can be useful, because of the order of operations. In your example, if an exception is thrown, code is executed in the order inner-try, finally, catch, which means that the exception handler is called after the finally has been called and cleaned up. In a standard try { ... } catch { ... } finally { ... }, the order is try, catch, finally, so exception handlers are called before cleanup. If your exception handler is logging, aborting execution or calling something which assumes that the data is in a good state, the former can be better.
Edit: also, Luc makes a good point, if the finally code throws an exception, this structure will catch it, though finally code should not throw exceptions unless it is truly unavoidable.
|
|
|
|
|
The outer try/catch in this example is pointless, remove it.
If the catch in the outer try/catch actually does something (like log the Exception), then the inner try/finally should be removed and the finally moved out to make the try/catch into a try/catch/finally.
Nested try/catches (while sometimes necessary) are a code smell and should be investigated thoroughly. (Or Thoreau[^]ly -- simplify simplify.)
|
|
|
|
|
PIEBALDconsult wrote: If the catch in the outer try/catch actually does something (like log the Exception), then the inner try/finally should be removed and the finally moved out to make the try/catch into a try/catch/finally.
If the catch in the outer try/catch actually does something, then your suggestion would change what happens to exceptions thrown by the finally block itself.
Luc Pattyn [Forum Guidelines] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, improve readability, and make me actually look at the code.
|
|
|
|
|
Luc Pattyn wrote: exceptions thrown by the finally block itself.
Of which there are none. or put a try/catch in the finally.
|
|
|
|
|
PIEBALDconsult wrote: Of which there are none
Of course there are, that is exactly what the three dots are standing for. You can't escape nested try constructs if it has to be fool proof...
Luc Pattyn [Forum Guidelines] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, improve readability, and make me actually look at the code.
|
|
|
|