Refactoring an existing MVC codebase to remove business logic & data access from the Controllers

Question

Product I've inherited has the following composition:

• MVC web site where the controllers make calls to Repositories to get, insert and update objects retrieved that are

defined in the:

• Data access layer written using Linq2SQL

This means that currently the controller for the action /Account/Add will perform the following pseudo-code:

• Get the current user
• new() an AccountItem object
• Set the UserId, AccountItemTypeId and Value fields on the AccountItem
• Get an instance of Repository
• Call the repositorys InsertOnSubmit(AccountItem item) method

• Call the repositorys SubmitChanges() method

• new() an EmailQueue item

• Set the appropriate properties on the EmailQueue item (to, subject, body, etc,..) based on the inserted account item
• Get an instance of Repository
• Call the repositorys InsertOnSubmit(EmailQueue item) method
• Call the repositorys SubmitChanges() method

I've already made a first pass through the codebase and injected, using Castle Windsor, the repository's in (actually injecting an instance of a type that implements IRepositoryResolver which has a method called GetRepository<T> so that I can decouple the controllers from the Linq2SQL dependent repositories. I've also started to write unit tests, based on the current behaviour of the controllers (to ensure that when I make further changes I can identify when I get it horribly wrong) but that's tangenital to my actual question.

I've identified lots of places where the controller actions are performing identical actions against various repositories, such as getting a given users total Value for all AccountItem's and am now considering extracting all this common logic into a business/service layer. My question is, would the following structure be considered "best practice":

• AccountService: Provides an AddAccountItem method (plus others such as GetAccountValue)
• EmailService: Provides an AddEmailToQueue method

Should I be refactoring the controller so it does:

accountServiceInstance.AddAccountItem(userToCredit, userCrediting, AccountItemType.Bonus, 300);
emailServiceInstance.AddEmailToQueue(userToEmail, EmailType.AccountCreditBonus);

Or, should my AccountService take an EmailService as a dependency (that's satisfied by Castle Windsor) and it's the responsibility of the AccountService to call EmailService.AddEmailToQueue?

Answer 1

If the business rule is that AddAccountItem must AddEmailToQueue every time it is successful then I would use the dependency. Otherwise you will have to make sure you wire it up every time.

I feel that email rules are business logic not presentation logic and would keep it out of the controller.