Does This Use Of Exceptions Smell Bad?

Yii 1.1 General Discussion
1

After posting a response in the RESTful API thread showing a pattern I use to handle updates, I got a bit self-conscious about my use of Exceptions for handling action execution. I typically use exceptions to log errors and relay friendly/helpful messages to the end user when something goes wrong.

For example, I use a special ValidationException class to 1) Decide whether to display a flash message to users performing a CRUD operation, and 2) to track which models and attributes are confusing my end users (by generating a lot of validation exceptions).

I’ve attached a code example below. My basic question is, does my code look horrible? What are the drawbacks to this method? I generally work in an isolated developer environment (small firm) so I don’t get a lot of (read: any) feedback on style. All feedback is appreciated!




public function ActionUpdate($id) {

	$customer = Customer::model()->findByPk($id);

	if ($customer===null)

		throw new CHttpException(404, "Customer #{$id} was not found in the database");

	elseif (!Yii::app()->user->checkAccess('Customer.update', array('modelId' =>$id)))

		throw new CHttpException(403, "You are not authorized to update customer #{$id}");


	if (isset($_POST['Customer'])) {

		$customer->setAttributes($_POST['Customer']);


		try {

			if (!$customer->validate())

				throw new ValidationException($customer);

			elseif (!$customer->save(false))

				throw new SaveException($customer);


			Yii::app()->user->setFlash("success", "Hooray! Customer #{$id} updated");

			$this->refresh();

		} catch (ValidationException $e) {

			Yii::app()->user->setFlash("warning", "Oh no, you entered bad data");

		} catch (SaveException $e) {

			Yii::app()->user->setFlash("error", "Oh dear, the application failed!");

		}

	}


	$this->render('update', array('customerModel' =>$customer));

}
2

To be honest i can’t see the advantage of this pattern. For one, validation errors are not application errors but rather a common state in an application. So i’d never use exceptions for that. Apart from that you only add more complexity in your example. Without exceptions you can achieve exactly the same with less code.

3

You definitely won’t achieve the same with less code:

  • Exception classes can perform automatic logging/tracking that would be tedious to track otherwise. I use mine to log the model, attribute, and user that triggered the error so that sales/support can potentially follow up with help. Maybe there’s a better way?

  • Validation/Save exceptions are basically CHttpException(400 / 500), which comes in handy when relaying messages back to Ajax. (Use the error handler to reformat them and set headers.)

For case (2), I’m starting from an assumption that my client-side guys are going to want to know exactly what happened to their Ajax requests. They’ll have code that looks something like this:




// Granular

$.ajax({

	statusCode: {

		400: function(jqXHR, statusText, errorThrown) {/* validation */},

		403: function(jqXHR, statusText, errorThrown) {/* forbidden */},

		404: function(jqXHR, statusText, errorThrown) {/* not found */},

		500: function(jqXHR, statusText, errorThrown) {/* save error */}

	}

});


// General

$.ajax({

	// stuff

}).done(function(){

	// success

}),fail(function(){

	// error

});

Using this pattern you can easily return helpful, natural error messages to the client-side guys.

As for validation errors being a common application state, I view them as getting in the way of any PUT/POST request fulfilling its promise. That’s a natural spot for an exception telling the client: “You sent me bad information; I need you to fix that and re-submit.”

4

I’m with Mike here. While your code does not really qualify as bad, I certainly feel exceptions were set in place in an abusive way. I’m also not quite sure how the logging of validation errors works for you. How do you determine if “putting a character in a field that should only take numbers” is something that should get attention from support?

Oh yes, as a side note: The controllers generated by gii will throw a 404 if you try to load a model via [font=“Courier New”]FooController.loadModel($id)[/font] that doesn’t exist.

5

Seems a bit overkill. A possibly better approach, at least for tracking which fields are consistently invalid, would be to create a behavior or component that listens to a onValidationFailed event. This is reusable, can be turned on/off quickly, and doesn’t clutter the controller’s update/create actions.

6

On reflection, I’m definitely mixing two use cases:

1) Human Clients

This is your typical web user. Here you’re going to let the user pick up where they left off anyway, so it makes a lot less sense to use exceptions. This is the use case I think everyone is thinking about. (My example didn’t help.)

2) Computer Clients

An example might be an Ajax request to your API. In this case you want to:

[list=1]

[*]Stop application flow

[*]Set a sensible header

[*]Relay error info in a consistent machine-readable format

[/list]

Given Yii’s default handling of CHttpExceptions, exceptions are pretty much perfect here. No?

In both cases you might run into situations where a certain result (error) really does require an immediate response, e.g. manually rolling back transactions. Exceptions make sense here as well.

This has been referenced a few times but I’ve not heard why.

Is it about expectations? Mike mentioned that he expects validation errors - so do I. But I think what matters is what the application ‘expects’. In that case, bad attribute data really is exceptional because it means the current request can’t be fulfilled.

Is it readability? Personally, I think try{} catch{} is more readable than nested IFs. To follow nested IFs you pretty much need comments, whereas well-named exceptions in a try-catch naturally let you know what went wrong:




try {

	// Perfect world

}

// What can go wrong

catch (SpecificProblem1 $e) {...}

catch (SpecificProblem2 $e) {...}

Sometimes sales or support staff will argue that an application feature is too complicated and should be redesigned. By tracking this we can quickly test if they have a point (more validation errors than expected + high volume) or if their sample is biased. It’s mainly a way to streamline internal processes and improve time/resource allocation. (Plus we can cross-reference with what we know about the customer, e.g. country/language/internet speed.)

Agreed!

I don’t think you even need a custom event: afterValidate() gets called regardless of what happens in validate(), so it’s sufficient just to add a behavior method beforeValidate() that checks $event->sender->getErrors().