HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Registration and Login: Best way to separate responsibilities

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
registrationresponsibilitiesloginwayseparateandbest

Problem

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);
customerServi

Solution

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.

Code Snippets

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);
}

Context

StackExchange Code Review Q#69612, answer score: 3

Revisions (0)

No revisions yet.