views:

1125

answers:

5

I'm currently writing an ASP.Net app from the UI down. I'm implementing an MVP architecture because I'm sick of Winforms and wanted something that had a better separation of concerns.

So with MVP, the Presenter handles events raised by the View. Here's some code that I have in place to deal with the creation of users:

public class CreateMemberPresenter
{
 private ICreateMemberView view;
 private IMemberTasks tasks;

 public CreateMemberPresenter(ICreateMemberView view) 
  : this(view, new StubMemberTasks())
 {   
 }

 public CreateMemberPresenter(ICreateMemberView view, IMemberTasks tasks)
 {
  this.view = view;
  this.tasks = tasks;

  HookupEventHandlersTo(view);
 }

 private void HookupEventHandlersTo(ICreateMemberView view)
 {
  view.CreateMember += delegate { CreateMember(); };
 }  

 private void CreateMember()
 {   
  if (!view.IsValid)
   return;  

  try
  {
   int newUserId;
   tasks.CreateMember(view.NewMember, out newUserId);
   view.NewUserCode = newUserId;
   view.Notify(new NotificationDTO() { Type = NotificationType.Success });    
  }
  catch(Exception e)
  {
   this.LogA().Message(string.Format("Error Creating User: {0}", e.Message));
   view.Notify(new NotificationDTO() { Type = NotificationType.Failure, Message = "There was an error creating a new member" });
  }
 }
}

I have my main form validation done using the built in .Net Validation Controls, but now I need to verify that the data sufficiently satisfies the criteria for the Service Layer.

Let's say the following Service Layer messages can show up:

  • E-mail account already exists (failure)
  • Refering user entered does not exist (failure)
  • Password length exceeds datastore allowed length (failure)
  • Member created successfully (success)

Let's also say that more rules will be in the service layer that the UI cannot anticipate.

Currently I'm having the service layer throw an exception if things didn't go as planned. Is that a sufficent strategy? Does this code smell to you guys? If I wrote a service layer like this would you be annoyed at having to write Presenters that use it in this way? Return codes seem too old school and a bool is just not informative enough.

+1  A: 

That sounds just right to me. Exceptions are preferable as they can be thrown up to the top of the service layer from anywhere inside the service layer, no matter how deeply nested inside the service method implementation it is. This keeps the service code clean as you know the calling presenter will always get notification of the problem.

Don't catch Exception

However, don't catch Exception in the presenter, I know its tempting because it keeps the code shorter, but you need to catch specific exceptions to avoid catching the system-level exceptions.

Plan a Simple Exception Hierarchy

If you are going to use exceptions in this way, you should design an exception hierarchy for your own exception classes. At a minumum create a ServiceLayerException class and throw one of these in your service methods when a problem occurs. Then if you need to throw an exception that should/could be handled differently by the presenter, you can throw a specific subclass of ServiceLayerException: say, AccountAlreadyExistsException.

Your presenter then has the option of doing

try {
  // call service etc.
  // handle success to view
} 
catch (AccountAlreadyExistsException) {
  // set the message and some other unique data in the view
}
catch (ServiceLayerException) {
  // set the message in the view
}
// system exceptions, and unrecoverable exceptions are allowed to bubble 
// up the call stack so a general error can be shown to the user, rather 
// than showing the form again.

Using inheritance in your own exception classes means you are not required to catch multipile exceptions in your presenter -- you can if there's a need to -- and you don't end up accidentally catching exceptions you can't handle. If your presenter is already at the top of the call stack, add a catch( Exception ) block to handle the system errors with a different view.

I always try and think of my service layer as a seperate distributable library, and throw as specific an exception as makes sense. It is then up to the presenter/controller/remote-service implementation to decide if it needs to worry about the specific details or just to treat problems as a generic error.

Cheekysoft
+1  A: 

As Cheekysoft suggests, I would tend to move all major exceptions into an ExceptionHandler and let those exceptions bubble up. The ExceptionHandler would render the appropriate view for the type of exception.

Any validation exceptions however should be handled in the view but typically this logic is common to many parts of your application. So I like to have a helper like this

public static class Try {
    public static List<string> This( Action action ) {
      var errors = new List<string>();
      try {
        action();
      }
      catch ( SpecificException e ) {
        errors.Add( "Something went 'orribly wrong" );
      }
      catch ( ... )
      // ...
     return errors;
    }
}

Then when calling your service just do the following

var errors = Try.This( () => {
  // call your service here
  tasks.CreateMember( ... );
} );

Then in errors is empty, you're good to go.

You can take this further and extend it with custome exception handlers which handle uncommon exceptions.

tgmdbm
A: 

Cheekysoft, I like the concept of a ServiceLayerException. I already have a global exception module for the exceptions that I don't anticipate. Do you find making all these custom exceptions tedious? I was thinking that catching base Exception class was a bit smelly but wasn't exactly sure how progress from there.

tgmdbm, I like the clever use of the lambda expression there!

Thanks a lot guys, I at least feel that I'm on the right path.

Scott Muc
+1  A: 

In reply to the follow-up question:

As for creating exceptions becoming tedious, you kinda get used to it. Use of a good code generator or template can create the exception class with minimal hand editing within about 5 or 10 seconds.

However, in many real world applications, error handling can be 70% of the work, so it's all just part of the game really.

As tgmdbm suggests, in MVC/MVP applications I let all my unhandlable exceptions bubble up to the top and get caught by the dispatcher which delegates to an ExceptionHandler. I set it up so that it uses an ExceptionResolver that looks in the config file to choose an appropriate view to show the user. Java's Spring MVC library does this very well. Here's a snippet from a config file for Spring MVC's Exception resolver - its for Java/Spring but you'll get the idea.

This takes a huge amount of exception handling out of your presenters/controllers altogether.

<bean id="exceptionResolver"
      class="org.springframework.web.servlet.handler.SimpleMappingExceptionResolver">

  <property name="exceptionMappings">
    <props>
      <prop key="UserNotFoundException">
        rescues/UserNotFound
      </prop>
      <prop key="HibernateJdbcException">
        rescues/databaseProblem
      </prop>
      <prop key="java.net.ConnectException">
        rescues/networkTimeout
      </prop>
      <prop key="ValidationException">
        rescues/validationError
      </prop>
      <prop key="EnvironmentNotConfiguredException">
        rescues/environmentNotConfigured
      </prop>
      <prop key="MessageRejectedPleaseRetryException">
        rescues/messageRejected
      </prop>
    </props>
  </property>
  <property name="defaultErrorView" value="rescues/general" />
</bean>
Cheekysoft
A: 
Scott Muc