Try/catch control flow
-
So sometimes I like to use try/catch for control flow.
I've read a few articles about this and it seems to me most criticisms come down to "I once read that GOTO is bad and this is like local GOTO so this is bad too".
Fuck you. You know what else is like GOTO? Pretty much any other control structure there is. The article most people are referring to for their anti-GOTO dogma was written like 40 years ago. Back then
if / while / for
control structures were just being introduced and Dijkstra was railing against the terrible GOTO-pasta programming style of his time. No one's going back to that, even if we could. Luckily it seems lately the pendulum has been swinging back towards sanity on this front.But ok, let's get back to try/catch. The situation I most often use try/catch for control flow is in form validation. User submits a form. You need to do basic sanity checks / data coercion, then a deeper business rules validation and finally, try to persist it into database. Each of these steps can produce a failure state. Some of them need to be displayed to the user ("Name already exists"). Some are potential bugs and need to produce a 550 page and a log entry. Example:
First, define a few custom Exception types
public class MyException : Exception { public MyException(string message = null) : base(message) { } public MyException(Exception ex) : base(ex.Message, ex) { } } public class DisplayException : MyException { public DisplayException(string message = null) : base(message) { } public DisplayException(BLException ex) : base(ex) { } }
Then, handle user's POST. For example, in WebForms Save() method
public void Save() { try { // Sanity checks if (this.Name == null || this.Date == null) { throw new DisplayException("Invalid post data"); } int code; if (!int.TryParseInt(this.Code, out code)) { throw new DisplayException("Code must be a number"); } var item = new BLItem { Name: this.Name, Date: this.Date, Code: code }; // Validate and save. Throws its own exceptions try { BLManager.Save(item); } catch (BLException ex) { // Convert thrown BL exception into a type // used for control flow in the interface throw new DisplayException(ex); } this.DisplayMessage("Data saved", MessageType.Success); } catch (Exception ex) { if (ex is DisplayException) { // We can just display what happened to the user this.DisplayMessage(ex.Message, MessageType.Danger); } else { // This wasn't user's fault. Log it for later this.LogException(ex); this.RedirectToCode(550); } } }
I didn't include that, but BLManager would use similar syntax to convert database's
DuplicateKeyError
into some kind of "Item with name xxx already exists
" error.So, what do you think? Am I the real WTF?
-
It's okay... ish in that example. Invalid data is kind of an exceptional situation.
if (ex is DisplayException)
irks me a little, since you could just use two catch blocks.And
DisplayException
is kind of a silly name. What if later you decide that you're not going to display those exceptions, but do something else with them?
-
I don't see a problem with that, although the catch should be separated into two blocks, assuming your language supports that.
Edit: ninja'd :(
-
if (ex is DisplayException) irks me a little, since you could just use two catch blocks.
In real life, there could be code that needs to happen in either case. And if I put it after the catch block, then I can't escape from method early. Meh, depends on case by case basis, I guess.
-
And DisplayException is kind of a silly name. What if later you decide that you're not going to display those exceptions, but do something else with them?
Yes I suppose you could change it to
UserException
or something. The point is, this is used only for control flow within UI, so refactoring isn't an issue.
-
In real life, there could be code that needs to happen in either case. And if I put it after the catch block, then I can't escape from method early. Meh, depends on case by case basis, I guess.
This suggests that exceptions may not be the ideal solution. They don't fit what you're trying to do.
-
I agree with Keith: If you are throwing an exception, and you can't catch it with your other exceptions because it's just data validation that you want to respond to, you might be better served making a return response for your calling query. Boolean for success/fail, int or string for various types of fails, list/arrays for potentially stacking issues, etc.
It's not necessarily wrong, but it's one of those 'ehhhh' type of items.
But why doesn't this work?
try
{
}
catch(DisplayException)
{
Your stuff
//No rethrow
}
catch(Exception)
{
Your stuff
Throw / redirect
}
-
Guys, two catch blocks are perfectly fine. It was just a stylistic choice here to use one. It doesn't change the gist of the pattern.
-
It's not too bad; the exceptions do represent error cases, and that's the main thing. Using them for non-errors would be nasty.
However, I'd want to refactor the code. Not to fundamentally change what you're doing, but rather to divide the logic of the parsing and accessing the database from the logic of displaying the results. The exceptions would flow across functional boundaries, but that's one thing they're designed to do.
private void SaveCore() { // Sanity checks if (this.Name == null || this.Date == null) { throw new DisplayException("Invalid post data"); } int code; if (!int.TryParseInt(this.Code, out code)) { throw new DisplayException("Code must be a number"); } var item = new BLItem { Name: this.Name, Date: this.Date, Code: code }; // Validate and save. Throws its own exceptions try { BLManager.Save(item); } catch (BLException ex) { // Convert thrown BL exception into a type // used for control flow in the interface throw new DisplayException(ex); } } public void Save() { try { this.SaveCore(); this.DisplayMessage("Data saved", MessageType.Success); } catch (Exception ex) { if (ex is DisplayException) { // We can just display what happened to the user this.DisplayMessage(ex.Message, MessageType.Danger); } else { // This wasn't user's fault. Log it for later this.LogException(ex); this.RedirectToCode(550); } } }
I've only done the code shuffling here, and the name might suck ;) but I'm sure you get the idea. The inner method can be much stricter than the outer one; either satisfy its constraints or it throws the call out with an exception. It's much easier to understand the correctness of that.
(In my applications I'd actually try to do the outer function via AOP injection targeted via annotations as the rendering code is highly regular, but I use Java and I don't use C#, so I don't know that my real approach would carry over to you. Nor am I sure if you've got the regularity in your app anyway…)
Code nirvana is attained when the way it works is so obvious that someone else coming along to maintain it says “Well of course it works that way! How else could it be?” Little do they know what you went through to get there…
-
Fair enough, this is a common pattern in WebForms programming.
As for AOP, I have zero experience with that. I can tell that you can use C# attributes in place of annotations, but not sure about the pattern itself. It seems it isn't baken into the language as with Java and I personally have never seen it used.
-
[AOP] seems it isn't baken into the language as with Java and I personally have never seen it used.
It's implemented either during the compilation stage or via a code-rewriting runtime. I find it is a great way to implement wrappers of various kinds; exception rewriting, transaction management, logging, performance tracking.