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

1

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

answered Mar 31, 2011 at 3:00

BlackTigerX

6,1667 gold badges42 silver badges50 bronze badges

Collectives™ on Stack Overflow

ASP.NET MVC Controller Logic

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related