I fixed one controller action in my application that doesn’t seemed good enough for me. It wasn’t big move I did but worth to show to beginners how nice code you can write when using correct layering in your application. As an example I use code from my posting ASP.NET MVC: How to implement invitation codes support.
Problematic controller action
Although my controller action works well I don’t like how it looks. It is too much for controller action in my opinion.
[HttpPost]
public ActionResult GetAccess(string accessCode)
{
if (string.IsNullOrEmpty(accessCode.Trim()))
{
ModelState.AddModelError("accessCode", "Insert invitation code!");
return View();
}
Guid accessGuid;
try
{
accessGuid = Guid.Parse(accessCode);
}
catch
{
ModelState.AddModelError("accessCode", "Incorrect format of invitation code!");
return View();
}
using (var ctx = new EventsEntities())
{
var user = ctx.GetNewUserByAccessCode(accessGuid);
if (user == null)
{
ModelState.AddModelError("accessCode", "Cannot find account with given invitation code!");
return View();
}
user.UserToken = User.Identity.GetUserToken();
ctx.SaveChanges();
}
Session["UserId"] = accessGuid;
return Redirect("~/admin");
}
Looking at this code my first idea is that all this access code stuff must be located somewhere else. We have working functionality in wrong place and we should do something about it.
Service layer
I add layers to my application very carefully because I don’t like to use hand grenade to kill a fly. When I see real need for some layer and it doesn’t add too much complexity I will add new layer. Right now it is good time to add service layer to my small application. After that it is time to move code to service layer and inject service class to controller.
public interface IUserService
{
bool ClaimAccessCode(string accessCode, string userToken,
out string errorMessage);
// Other methods of user service
}
I need this interface when writing unit tests because I need fake service that doesn’t communicate with database and other external sources.
public class UserService : IUserService
{
private readonly IDataContext _context;
public UserService(IDataContext context)
{
_context = context;
}
public bool ClaimAccessCode(string accessCode, string userToken, out string errorMessage)
{
if (string.IsNullOrEmpty(accessCode.Trim()))
{
errorMessage = "Insert invitation code!";
return false;
}
Guid accessGuid;
if (!Guid.TryParse(accessCode, out accessGuid))
{
errorMessage = "Incorrect format of invitation code!";
return false;
}
var user = _context.GetNewUserByAccessCode(accessGuid);
if (user == null)
{
errorMessage = "Cannot find account with given invitation code!";
return false;
}
user.UserToken = userToken;
_context.SaveChanges();
errorMessage = string.Empty;
return true;
}
}
Right now I used simple solution for errors and made access code claiming method to follow usual TrySomething() methods pattern. This way I can keep error messages and their retrieval away from controller and in controller I just mediate error message from service to view.
Controller
Now all the code is moved to service layer and we need also some modifications to controller code so it makes use of users service. I don’t show here DI/IoC details about how to give service instance to controller. GetAccess() action of controller looks like this right now.
[HttpPost]
public ActionResult GetAccess(string accessCode)
{
var userToken = User.Identity.GetUserToken();
string errorMessage;
if (!_userService.ClaimAccessCode(accessCode, userToken,
out errorMessage))
{
ModelState.AddModelError("accessCode", errorMessage);
return View();
}
Session["UserId"] = Guid.Parse(accessCode);
return Redirect("~/admin");
}
It’s short and nice now and it deals with web site part of access code claiming. In the case of error user is shown access code claiming view with error message that ClaimAccessCode() method returns as output parameter. If everything goes fine then access code is reserved for current user and user is authenticated.
Conclusion
When controller action grows big you have to move code to layers it actually belongs. In this posting I showed you how I moved access code claiming functionality from controller action to user service class that belongs to service layer of my application. As the result I have controller action that coordinates the user interaction when going through access code claiming process. Controller communicates with service layer and gets information about how access code claiming succeeded.
View Comments (16)
out parameters looks just awful
About out parameter - it seemed like most effective way right now. I really didn't want to add some more complex error handling to my application right now. Like alwaya - I switch to another solution when there is need for it. :)
Good post, and I agree with using either a service class or manager class in the controller, and maybe put some repository or repository factory implementation inside your service / manager classes. The out parameter is not so awful in my opinion, especially in this case since it's not the point of your article to discuss how to present error messages. That could be another post ;-) to present some alternative ways for handling errors in your UI.
Those multiple exit points within your methods are making me twitch. Encapsulate that stuff into a data validation method that returns only a single result.
Multiple exits tells me that not enough thought went into the solution and I question how TDD is being used to provide the above example.
Hi Gunnar,
I recommend the service layer. >> "Controller is not the new code behind!!!"
But please...
- Don't write you own auth system/security code. We have membership.
- Don't use sessions. Apps written this way don't behave well. Its like global variables. It breaks stuff sooner or later and does not scale well.
--Daniel
Thanks for feedback, Daniel and Rob!
Daniel, my auth is purely based on WIF and therefore I don't have any custom auth mechanism in use. What I use here is just simple trick to let in only users who are invited to administer my system.
Rob, my winnings on keeping code on service layer can be huge. I've seen fat controller actions and they are pain to live with. Validation - yeah, right, it will be here but with some future blog post as my code evolves :)
Haha. It looks like the good 'ol put validation and error messages in a function to save space. Now this simple idea has been confounded with gobbly-gook mad scientist verbosity. Oh well, it's not your fault and your blogs do help me a lot.
Thin controllers are my current favorite. The code here is pretty old and over time I have improved the internals of service layer and communication interface. If presentation and service layers are separated then I can easily work on service layer without affection presentation layer. This has been huge benefit this far.
Thank you Gunnar. You are my new goto blog to read.
But aren't controllers already separated from presentation layer?
The View is not calling the "function" (read: service layer) but the Action in the controller is calling the service layer in order to make itself a Skinny Controller. no?
MVC pattern targets separation issues but only inside presentation layer. It's presentation layer pattern and it's not protected against higher lever architectural problems. The fact that controller and view are separated doesn't stop anybody to write domain logic to controllers and therefore pushing controller to two roles: controller and perverted container of business logic. I prefer controllers to be mediators between presentation layer and domain model but I don't want any business logic to be located in controllers.
This is nice and thanks for sharing. Personally I think ClaimAccessCode is a bad name because I have to go into it to see what it does. To me, the name implies that I'm claiming an access code. Instead it is validating an access code. How about IsValidAccessCode. This way your API reads much nicer, !_userService.IsValidAccessCode.
Just an opinion :)
Barry, the method claims access code after validation. Actually you just ointed out readibility issue that can be solved by moving validation to some other place :)
Hi Gunnar,
Sorry for the beginner question but I can't see how and where _userService gets declared and instantiated?
My current Controllers are way too fat. I feel that maybe the logic should be put into the viewmodels or beyond, into service layer between view models and datamodels.
How should I organize this?
Here I expect that UserService is injected to controller constructor:
public HomeController : Controller
{
private readonly IUserService _userService;
public HomeController(IUserService userService)
{
_userService = userService;
}
}
To inject instances you can use custom controller factory like one shown here: http://gunnarpeipman.com/2010/11/asp-net-how-to-avoid-argumentnullexception-in-structuremap-controller-factory/
Nice post Gunnar,
I am trying to make my controller look thin and if I want to follow Repository > Service > Presentation layer pattern, can I still put methods like your ClaimAccessCode() in Service Layer? Perhaps like ExcelExport function in service layer that does not access its repository. (To my understanding, services do little more than access a repository and return whatever the repository returns)
For example in service class what I usually see is that it accesses repository's methods:
_StudentRepository.Delete(student);
If I have a method that does have nothing to do with its repository, can I still put that in the service class? or should I put that in controller?
Elaborated link: http://stackoverflow.com/questions/43553019/repository-pattern-and-service-layer
Service class uses repository only if it implements use-case where data from repository is needed. It's possible to have service class that instead communicates with some web services or devices or doesn't have any external communication at all.