Architecture / Design Pattern for Login Signup

Question

I have a multi-seller ecommerce platform, derived out of Opencart 2.3. We changed quite a bit of the core code of the Opencart to add our own customizations, including features such as Seller Dashboard (where a seller can check orders, generated invoices for them, etc etc). Repetition of code and lack of basic OOPs principles like Abstraction in Opencart, have been discussed quite in detail.

Now, as we scale up, we are looking at rewriting it. We are currently starting with Login/Signup module. A customer or a seller can login/signup using the same form. We will be using cookie based token validation for login. We will also be maintaining different tokens for different devices/browsers/Android Apps of the customer, so that he can remain logged in at one place, while logging out from the other. Depending on whether a customer/seller logs in, it will redirect to appropriate page.

I am thinking of following Class structure:

Login Class {
  public login();
  public logout();
  public validate();
}
Signup Class extends Login Class {
  public login();
  public logout();
  public validate();
  public signup();
}

Login Controller will create object of Signup Class, and call login/signup/validate methods. Depending on the login/validate results, it will return appropriate object (Customer or Seller).

Am I thinking on the right lines ? Are there any Design Patterns to achieve this ?

Sorry, if question looks too broad or convoluted. Kindly let me know. I will be very happy to further elaborate the specific details of my case. Thanks in Advance.

Answer 1

Considerations from your questions and comments.

Tight Coupling:

You stated:

(1) Login Controller will create object of Signup Class
(2) A customer or a seller can login/signup using the same form.

(1) is tightly coupling two parts of the system in a way which they don't belong together.

(1) also contradicts the code example you have:

Signup Class extends Login Class 

As the two are opposite approaches.

I can understand the thought as to why they're related/joined as per (2), as you are using Login things in Signup together, and they have some real world similarity in that Signup and Login have some shared notions to the end user. You also want to check they're not logged in before signing them up.

For checking they're not logged in, this is possibly something that happens on all pages, so you would already know. But if not, then the Signup controller would call a separate service to find out if they are logged in. This other service would be re-used on various pages, so you define once in one class what is "logged in" and it's re-used throughout your code.

Classes do have dependencies all the time, bringing other objects in (eg via DI) however, in this case in the system these controllers are not directly related at all.

Bringing objects into a class is one thing and will often have some form of coupling. Classes will have dependencies from other parts of the system otherwise we'd not be able to adhere to SRP. But "extend" is a very tightly coupled relationship and shouldn't be seen as some kind of first port of call a developer should do unless there is very good reason, and then it's usually very obvious.

Classes can only extend one class. Signup has used that privilege with "Login Class". But doesn't Signup class need a validation class to validate the Signup form data? And a database class (eg a service calling a repository), to determine if the username is not in use? Etc.

So why extend Login class instead of database class? Well the answer is not "which is more important to choose which to extend" but "should this be coupled or not". Chances are you don't extend any of these, you would use composition.

A good but simple example of Composition
Also see Composition over Inheritance

 

Re your statement (1) - You cannot login before signing up, so Login has no business asking Signup to come to its party as then you're stating that Login is in charge of Signup, which is very wrong.

It would arguably be more logical to state the other way around "Signup gets a Login object" (re your code example), but in fact even Signup has no business asking Login to come to its party even when Signup is completed. This is because they are separate parts of the system, and Signup does not need Login to handle it's own single responsibility - Signup!

The requirements are separately "Signup" then "Login", so combining those specific requirements is "Login after Signup", not "use Signup to be able to eventually Login".

This said, Login and Signup should share some common ground from the system, such as using the same code for Login form and user credential validation, as these should be shared for re-use to maintain DRY. However such a common ground does not mean you should pin the controlling classes together like this (1) or even bring in an instance of the other's object.

In some cases some sort of mediator might make sense, that would bring together shared requirements and objects (such as a service), but in this case you'd just hand over to the next part of the system to log the user in after Signup.

In a different slightly stretched example just to reiterate the above point:

Say you send them to the UserProfile page after Login, you wouldn't bring the UserLogin object into the UserProfilePageController so that once Login is done the system is already in the profile controller ready to go.

The login system would be called, and then the login controller would hand over to whatever resource was requested - a page prior to Login that redirected them to Login, or simply the user profile page, etc. (Although this is more likely to be something else controlling this, front controller or internal wiring etc.)

 

---

Is tightly coupled so bad?

The problem with tightly coupling these things together with (statement 1) is when you just want to simply log someone in (they signed up months ago) your Login is still bringing in Signup object when you don't need it.

Or worse/equally as bad, you have to make a 2nd Login class just for pure Login that is not coupled to Signup. Then you have violated DRY as the two Login classes will be doing the same thing, they're just duplicated to (wrongly) cater for your bad design in coupling things.

The other issue is when you want to refactor the Login or the Signup you have to consider the other one because they're tightly coupled to each other's objects.

Of course such tight coupling can be valid, and dependencies are very much shared between classes. The key is good separation and de-coupling wherever possible means parts of the system run independent of each other and changes only effect that one part of the system.

 

---

The likely better approach:

Signup Request

1. Request and display SignupForm and LoginForm
2. Pass entered data to SignupFormValidation and LoginFormValidation
3. If errors show them [back to 1]
4. Register them
5. Hand over to the Login system (however your system does this, but not by having an instance of Login in the Signup class, probably a mediator service)

Login Request

1. Request and display LoginForm
2. Pass entered data to LoginFormValidation
3. If errors show them [back to 1]
4. Log them in

See how Signup and Login both call the same LoginForm and LoginFormValidation? This means (e.g.) if you decide that username can be 20 chars instead of 18, you make change in a centralised place (LoginForm and LoginFormValidation) and Signup and Login both inherit the same changes.

To log them in when they've signed up, I would suggest a mediator of some kind, such as a service. Probably called something like LoginAfterSignupService (I'm open to this being debated as there may be a better approach).

Signup will pass the username and password to this service who's job is to hand over to the Login system. It will pass the username/password (etc) to the Login system , which will just do it's normal thing and verify the username and password and log them in.

The login system will always need a username and password to verify the user, whether than comes from a login form or a mediator service that the signup system can call shouldn't matter. The login system shouldn't care where that comes from.

The reason I suggested a mediator service instead of Signup just handing over to the login system is because you need something to handle the specific case that, we're not on a login page, we're not on Signup, we're in the middle. So if login fails you are in a separate service who's job is solely to handle the middle ground of "login after signup" and can do some specific checks if login returns false (fails).

For example, login page would just state "sorry wrong username or password", but we've just signed up and that message would be confusing to the user. The specific service could instead handle it more appropriately (whatever would be appropriate depends on your system).

 

---

Quick note on your code in your question:

Sorry but this is a bad smell:

Login Class {
  public login();
  public logout();

"Login" class should not have a method "Logout". The code to use this would look awkward:

$login = new Login();
$logout = $login->logout();

// Method chaining would be worse
$logout = new Login()->logout();

Just make a separate logout class so you would do this:

$logout = new Logout();
$logout = $logout->logout();

// Method chaining would be nice to use
$logout = new Logout()->logout();

You may find now or in the future that your logout process changes, to sending an email, or checking something specific (etc etc). If this is a class these new requirements are valid methods in that class. If "logout" is a method in "login" class, apart from being a violation of SRP, you now have to stuff all kinds of "logout" requirements into a single "logout" method in the "login" class.

 

---

This is very roughly what I think it would look like

class SignUp
{
    public function __construct(
        IsUserLoggedIn $isUserLoggedIn,
        LogUserInAfterSignupService $logUserInAfterSignup,
        RegisterNewUserService $registerNewUser
    ) {
        $this->isUserLoggedIn = $isUserLoggedIn;
        $this->logUserIn = $logUserInAfterSignup;
        $this->registerNewUserService = $registerNewUser;
    }

    public function signupAction(
        SignupForm $signupForm,
        SignupValidation $signupValidation,
        LoginForm $loginForm,
        LoginValidation $loginValidation
    ) {
        if ($this->isUserLoggedIn() === true) {
            // Probably show a view page telling them they're already logged in
        }

        // If no form data, pass to view the $signupForm and $loginForm

        // If form data
          // Pass relevant data to $signupValidation and $loginValidation
          // Show errors if exist

        // If form data and no errors
          $userId = $this->registerNewUser($formData);
          $this->logUserIn($userId);

        // Redirect to wherever

    }

    private function isUserLoggedIn()
    {
        return $this->isUserLoggedIn->userLoggedIn();
    }

    private function registerNewUser($formData)
    {
        // Call a service that calls a repository that insert form data (etc)
       // registerNewUserService would return the new user ID
        return $this->registerNewUserService->registerNewUser($formData);
    }

    private function logUserIn($userId)
    {
        $this->logUserIn->logInUser($userId);
    }
}

You'd probs want some sanity checking in there, and throw some exceptions perhaps (or the services this class calls might throw them, like in the RegisterUserService and LogUserInAfterSignupService).