ASP.NET MVC Controller Logic

Question

I am new to this whole MVC stuff, so please bear with me.

I am wondering what is the correct way to implement controller logic.

In other words, take this very simple example, a forgot password screen. In traditional ASP/ASP.Net, this could eaisly be handled in one page, hide/show a div or two based on the flow... done!!

I have come up with the following and just wanted to see if I was on the right track. Have a look at the following controller:

Namespace Controllers Public Class AuthenticationController Inherits ControllerBase Private MembershipProvider As New GTGMembershipProvider <HttpGet()> Function LogOn() As ActionResult If (User.Identity.IsAuthenticated) Then Return RedirectToAction("Index", "Main") End If Return View(New LogOnViewModel) End Function <HttpPost()> Function LogOn(Model As LogOnViewModel, ReturnUrl As String) As ActionResult If (Not ModelState.IsValid) Then Return View(Model) End If If (Not MembershipProvider.ValidateUser(Model.UserName, Model.Password)) Then ModelState.AddModelError("", "Invalid login. Incorrect password/user name.") Return View(Model) End If IssueAuthenticationTicket(Model) If (Not ReturnUrl.IsNullOrEmpty) Then Return Redirect(ReturnUrl) Else Return RedirectToAction("Index", "Main") End If End Function Function LogOff() As ActionResult FormsAuthentication.SignOut() Return RedirectToAction("Index", "Main") End Function <HttpGet()> Function ForgotPassword() As ActionResult Return View(New ForgotPasswordViewModel) End Function <HttpPost()> Function ForgotPassword(Model As ForgotPasswordViewModel) As ActionResult If (Not ModelState.IsValid) Then Return View(Model) End If Return RedirectToAction("PasswordSent") End Function <HttpGet()> Function PasswordSent() As ActionResult Return View() End Function Private Sub IssueAuthenticationTicket(Model As LogOnViewModel) Dim Profile As New CustomerProfile With {.FirstName = "Sam", .ID = 1, .LastName = "Striano"} Dim Ticket As New FormsAuthenticationTicket(1, Model.UserName, Now, Now.AddDays(30), Model.RememberLogon, Profile.ToString) Dim Cookie As New HttpCookie(FormsAuthentication.FormsCookieName, FormsAuthentication.Encrypt(Ticket)) HttpContext.Response.Cookies.Add(Cookie) End Sub End Class End Namespace

you might want to check out codereview.stackexchange.com in the future for questions of this nature. — smartcaveman
– smartcaveman, Commented Apr 3, 2011 at 4:45

smartcaveman · Accepted Answer · 2011-03-31 02:54:54Z

You are on the right track. Three suggestions I have to make your code better are:

Refactor IssueAuthenticationTicket to method on an IAuthenticationService implementation, where IAuthenticationService looks like

public interface IAuthenticationService{ bool Authenticate(LogOnViewModel viewModel, HttpContextBase httpContext); }

That way your concerns are separated, so if you have to change the way the session is authenticated, you won't have to changed the controller logic. You could actually even keep your MembershipProvider dependency in the implementation and make the interface look like:

public interface IAuthenticationService{ bool Authenticate(LogOnViewModel viewModel, HttpContextBase httpContext, out string message); }
Look into Inversion of Control and Dependency Injection to inject your dependencies automatically into the controller via the constructor. My favorite tool for this is StructureMap. This way you don't have to couple the controller to a specific implementation and you don't have to handle any creation logic from within the controller. (The only thing this would affect in your sample would be the call to create a New GTGMembershipProvider. This isn't a big deal here, but with more complicated controllers with more complex dependencies, this could really take up a lot of space and make your code less readable).
You COULD put the logon model validation logic in a ModelValidator. But, this may or may not be a good idea, because it requires multiple instances of the MembershipProvider. If your membership provider is implemented as a singleton then this would make sense.

Sorry about the C# - I figure it's better to write it right in C#, then maybe write it right in VB.

BlackTigerX · Accepted Answer · 2011-03-31 03:00:28Z

I think this is good for being the authentication controller, some people migth suggest to create a level of abstraction and move code to another class, but in this case I don't think is necessary, for other controllers where you do deal with your business objects it will be necessary, the rule of thumb (IMHO) is that the only function the controller actions should perform is to gather the data for the model that will be displayed and passed to the view, how you get that data should be in other classes, but again, in this particular controller, I think this is simple enough to keep it this way

Collectives™ on Stack Overflow

ASP.NET MVC Controller Logic

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related