Registration and Login: Best way to separate responsibilities

Question

Based on lots of read books, sometimes I get quite confused when things get bigger than the simple examples. Martin Fowler's clean code is good but not enough.

I want some feedback on how to improve my current coding workflow, especially regarding to methods doing one thing or what is exactly one thing, does it depends on context. I'll show you an example:

Let's say I have these simple features:

SIGNOUT Whenever a visitor signs out:

• If the visitor is already logged in, clear user session and cookie data.

LOGIN Whenever a visitor logs in:

• Signout
• If there's session data assign current session data (like current cart to the logged in user)

REGISTER When a visitor submits the register form:

• Signout
• If the email already exist try to login with the password from registration form.
• If login failed return email in use error.
• If email does not exist create user, create profile info and send welcome email
• Login

Controller would have something like:

[HttpPost, ValidateModelState, ExportModelStateToTempData, ValidateAntiForgeryToken]
public RedirectResult Register(AccountRegisterBindingModel binding, string returnUrl) {
    var status = customerService.Register(binding);
    if (!status.Succeded) {
        ModelState.AddModelErrors(status.Errors);
        return Redirect(Url.Action("Register", "Account", new { returnUrl }));
    }
    return Redirect(returnUrl ?? DefaultUrl);    
}

As you can see, the controller is really thin, but I'm worried about the customerService.Register().

• First: name suggests to be: SingoutCreateUserSendWelcomeEmailLoginAndMergeSessionData()
• Second: Does this method do too many things? It looks like it.

The Register method have something like:

public SignInStatus Register(UserRegistration data) {
    authService.SignOut();
    var result = userService.CreateUser(data); // User and profile....
    if (result.Succedded) {
        communicationService.SendWelcomeEmail(data);
        customerService.MergeSessionCustomerWithUserCustomer();
    }
    return authService.Login(data)
}

Why I'm asking for a code review:

• While this may seem ok, the register method does too many things.
• I know I can put the logic pieces on the controller, but I have multiple endpoints for registration and I don't want to have duplicated code on controllers. Also everybody goes against it.
• This method is used on multiple websites (not just one) so change the code in the future can impact a lot. that smell like something is not quite good.
• If one consumer of this method changes I might have to use that in all or create a new one.
• This codebase does changes a lot, constant development of features, etc.

I hope it's clear enough. In reality, the code is more complex and nested, even if I try to keep it clean I sometimes don't know where to put the code pieces to keep it as clean as possible. While this is about a simple feature, I would love to get suggestions that will work for other parts of the software.

Answer 1

As I read the code, it seems that what the Register method does is control the business flow as defined in the specifications.

Since all it does (at least as it is written here) is delegate the actual actions to other methods, I don't think 'it does too much' - its single responsibility is to control the flow of the registration process.

To answer your concerns about future maintenance, I see two possible kinds of new features - features that should affect all the service consumers, and features that affect only a single (or part) of the consumers, but not all.

In the first case - there is no problem - do your change in the code, and it will affect all consumers (if you move the code to the controllers that won't be true, since you'd need to change all the controllers...)

In the second case, it should be easy enough to create a new register method for that use case. Since all it does is delegate, the new method itself will be just as simple as the old one, with maybe a delegation to a new method, something like this:

public SignInStatus RegisterAsAdmin(UserRegistration data) {
    authService.SignOut();
    var result = userService.CreateUser(data); // User and profile....
    if (result.Succeeded) {
        result = userService.MakeAdmin();
    }
    if (result.Succeeded) {
        communicationService.SendWelcomeEmail(data);
        customerService.MergeSessionCustomerWithUserCustomer();
    }
    return authService.Login(data);
}

Then connect the relevant consumers to the new service.

Answer 2

Style

Use guard clauses to reduce horizontal spacing.

Also there is no guidline, almost every c# developer would expect the opening brace { to be on the next line.

taking this into account the Controller.Register() method would look like

[HttpPost, ValidateModelState, ExportModelStateToTempData, ValidateAntiForgeryToken]
public RedirectResult Register(AccountRegisterBindingModel binding, string returnUrl) 
{

    var status = customerService.Register(binding);
    if (status.Succeded) { return Redirect(returnUrl ?? DefaultUrl); }

    ModelState.AddModelErrors(status.Errors);
    return Redirect(Url.Action("Register", "Account", new { returnUrl }));
}  

But let us now focus on the CustomerService.Register() method.

public SignInStatus Register(UserRegistration data) {
    authService.SignOut();
    var result = userService.CreateUser(data); // User and profile....
    if (result.Succedded) {
        communicationService.SendWelcomeEmail(data);
        customerService.MergeSessionCustomerWithUserCustomer();
    }
    return authService.Login(data)
}  

Here I would extract the administration of the user data to a separate method, because this is only done if result.Succedded evaluates to true.
While we are at result.Succedded is the naming difference status.Succeded wanted ?

So let us create

private void AdministrateUserRegistration(UserRegistration data)
{
    communicationService.SendWelcomeEmail(data);
    customerService.MergeSessionCustomerWithUserCustomer();
}

public SignInStatus Register(UserRegistration data) 
{
    authService.SignOut();
    var result = userService.CreateUser(data); // User and profile....
    if (result.Succedded) 
    {
        AdministrateUserRegistration(data);
    }
    return authService.Login(data)
}

Answer 3

There is a lot to go over here and I will try to be as thorough as I can. This is a complex issue about which there is much debate and so I will share my experience to apply some pragmatism as well as go over the logic (dogma?) behind the purpose of SRP, encapsulation, and many of these other concepts.

Firstly

Well done in reading / researching these topics. Too often developers focus on getting the solution and moving on. In software there is something to be said for problem solving (as that is what we are paid for) but another part that is at least as important, is writing code that makes sense and can be maintained. Good job then on asking.

Code

Here are some code smells that I noticed in your source listing.

---

(Side Note: It seems like you followed the example here: http://weblogs.asp.net/rashid/asp-net-mvc-best-practices-part-1 for your ExportModelStateToTempData method. I would NOT consider this even a GOOD practice for reasons I will elaborate on as we go through this answer, but that is my opinion)

---

In your Controller:

1) ModelState and Redirection

If the service call fails, you use the ModelState to add the errors returned by the service call. While this normally is what I would expect to see, your next line of code redirects the user to the Register action, which will lose the errors you have just added to the ModelState. It would seem as though you use the ExportModelStateToTempData attribute to persist the ModelState errors to the redirected page (TempData uses Session state). I had to read over this several times to notice this and while this doesn't fit the standard definition of a side-effect, it is at the very least difficult to read / understand. As a personal note, I don't care for TempData for several reasons I have found in my own coding:

• It's not strongly typed (more difficult to test / debug)
• It uses session under the covers (requires a lot of infrastructure if used in a web-farm)
• It encourages lazy coding standards (as do ViewBag / ViewData)
• Using an attribute to "hide" this functionality will lead to difficulties debugging and comprehending the code's function (and frankly there are much better ways of persisting state)

2) Var keyword

You use var to store the result of the call to the customerService.Register method. Since you know what the return type will be, you can increase the readability and maintainability by skipping using the var keyword like this:

 SignInStatus status = customerService.Register(binding);

There are some other gotchas with the var keyword as well, outlined here: https://stackoverflow.com/questions/356846/will-using-var-affect-performance.

I like your use of the DefaultUrl property (a commonly overlooked feature) and your use of the null coalescing operator (??) in the Redirect, that helps to keep the code very concise.

---

In your Service:

Just to clarify: In your Register method, the parameter is of type UserRegistration but as far as I can work out, when called from your controller, the controller passes a type of AccountRegisterBindingModel. Does the AccountRegisterBindingModel type derive from UserRegistration?

1) SRP This is in direct answer to your question: "Does this do too much?" -- Yes

As you correctly identified, Register is not a sufficient name for what this does and registration is itself an atomic process and should be handled as such. Here is my reasoning:

• Signing out should be an atomic operation performed elsewhere in the flow of the website. Under what circumstances could a signed-in user register for a new account? Given this restriction, the sign-out process should already have been accounted for and if it hasn't, that is the code smell that needs to be addressed, not having a "last-ditch" attempt to sign the user out in the register method. This can and should be removed from Register.

• The userService.CreateUser(data) call absolutely belongs here, though I would use the same reasoning about the var keyword I used above.

• The check for result.Succeeded, the word is spelled incorrectly, it should be Succeeded, not Succedded (I'm assuming this is a typo in your question, not the actual code).

• This check does nothing with a failure. In the controller you add errors, but I don't see any code that communicates what those errors are or how the Errors property referenced in your controller is filled with the relevant data.

• The communicationService.SendWelcomeEmail call is pretty gray area. I would argue that Registration is a "business" operation and therefore "business" operations apply here (login / logout for example are not business operations, but SendWelcomeEmail is a "business" operation).

• For the MergeSessionCustomerWithUserCustomer I'm confused. I thought the Register method was in the CustomerService, so why in the Register method do you refer to customerService as an external dependency as opposed to referring to this.MergeSessionCustomerWithUserCustomer()?

• As for login, it shouldn't be here. The website should after the registration occurs offer a link (or a checkbox prior to completion) that specifies "log me in when I'm done" or "Thnaks for creating an account, sign in now..." or something to that effect. This will have the result of removing this from the Register method.

• With that in mind, the return type of the Register method is one of SignInStatus... This smells funny. Why should my register also SignIn, shouldn't it instead return a RegistrationStatus?

• Finally, I wouldn't call the MergeSessionCustomerWithUserCustomer method here at all. This seems like something that should be done from the controller as part of a login (as it relates to session, which is a website concept, not a business concept).

---

With the register method free of non-related calls, any other consumer of this service can "Register" a user without unintended consequences. Without doing this, there are problems.

Let's say I wanted to write a console application that bulk-read in an excel or flat file of user registrations from a legacy system. I'd like to be able to use the CustomerService, but I can't because the register method produces side-effects (or worse, I use it not knowing about the side-effects).

So, to summarize, I would write the code like this:

In the controller:

[HttpPost, ValidateModelState, ValidateAntiForgeryToken]
public RedirectResult Register(AccountRegisterBindingModel binding, string returnUrl) {
    //Check to see if user is already logged in...
    //if (isAlreadyLoggedIn) Do something...

    RegistrationStatus status = customerService.Register(binding);
    if (!status.Succeded) {
        ModelState.AddModelErrors(status.Errors);
        return View("MyformWithErrors");
    } else {
        customerService.MergeSessionCustomerWithUserCustomer();
        return Redirect(Url.Action("Register", "Account", new { returnUrl }));
    }
    return Redirect(returnUrl ?? DefaultUrl);    
}

In your service:

public RegistrationStatus Register(UserRegistration data) {
    RegistrationStatus result = userService.CreateUser(data);
    if(result.Succeeded) {
        communicationService.SendWelcomeEmail(data);
    }
}

Really what you want is code that doesn't have side effects and only ever has ONE REASON to change (bug fixes). Following SOLID principles is very helpful but you also have to develop a keen nose for code smells.

I would suggest writing some unit tests for the methods and asking yourself these questions:

• How much setup is required to test method 'x'?
• How many test cases are required to thoroughly test method 'x'?
• How likely is it that a change to type 'y' will cause me to have to modify method 'x' or the tests for method 'x'?
• Does this test accurately describe the expected behavior of method 'x'?

Ideally, you want minimal setup, with only a few test cases. You also want the tests to not be 'brittle' and require modification whenever a dependency is changed and the tests should serve as the primary documentation for what the method does and what it's expected use is, which helps with re-use later.