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 :(



  • @Maciejasjmj said:

    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.



  • @Maciejasjmj said:

    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.



  • @cartman82 said:

    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.


  • Discourse touched me in a no-no place

    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.


  • Discourse touched me in a no-no place

    @cartman82 said:

    [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.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.