HTTP redirects after login, registration, and logout

Question

I am using following classes/interface to redirect user (after login, register, logout etc.)

File: RedirectInterface.php

interface RedirectInterface
{
    public function getUrl($customerId = null);
}

File: LoginRedirect.php

class LoginRedirect implements RedirectInterface
{
    public function getUrl($customerId = null)
    {
        // do some business logic here to get url
        $url = '/account/some-customer';
        return $url;
    }
}

File: RegisterRedirect.php

class RegisterRedirect implements RedirectInterface
{
    public function getUrl($customerId = null)
    {
        // do some business logic here to get url
        $url = '/welcome/some-customer';
        return $url;
    }
}

File: RedirectFactory - Creational design pattern (static factory)

class RedirectFactory
{
    public static function build($type, $customerId)
    {
        if ($type == 'login') {
            return new LoginRedirect($customerId);
        } else if ($type == 'register') {
            return new RegisterRedirect($customerId);
        }
        throw new InvalidArgumentException ('Invalid redirect type.');
    }
}

Usage:

// 1. Usage: Somewhere in customer registration code
$redirectUrl = RedirectFactory::build('register', 102)->getUrl();
header('Location: ' . $redirectUrl);

// 2. Usage: Somewhere in customer login code
$redirectUrl = RedirectFactory::build('login', 102)->getUrl();
header('Location: ' . $redirectUrl);

If you were given a chance to refactor this code. How would you have re-developed it?

Answer 1

• Your RedirectInterface type doesn't redirect. It's just a URL builder. Either the name should be changed to reflect that, or RedirectInterface needs a redirect method.

• You're supplying a customer ID to RedirectFactory::build, but your interface also specifies an optional $customerId parameter. If i'm ever handed a RedirectInterface, i can't make assumptions about how it was built. I don't know whether it was constructed with a customer ID, or whether i need to give it one -- or, even worse, whether the customer ID i supply will be used at all, or the one supplied to RedirectFactory::build will override it. I would get rid of the parameter in one place or the other to minimize that confusion.

• RedirectFactory does one of exactly two "redirects", and uses a string to decide between them. Right now, this is false indirection. You could replace these two cases with distinct functions, and no functionality whatsoever would be lost. (Think about it...are you ever going to not know which kind of redirect you want?)

  If you're going to keep this at all, then have an array mapping types to builder functions/objects for the various URL types. Provide a way to add redirect types to the array, and add your 'login' and 'register' cases in that manner. Something like, say,

  class RedirectFactory {
      private static $urlBuilders = [];
  
      public static function addRedirectType($type, RedirectInterface $builder) {
          if (isset(self::$urlBuilders[$type])) {
              throw new InvalidArgumentException("Type '$type' is already defined");
          }
  
          self::$urlBuilders[$type] = $builder;
      }
  
      public static function build($type) {
          if (isset(self::$urlBuilders[$type])) {
              return self::$urlBuilders[$type];
          }
          else throw new InvalidArgumentException('Invalid redirect type.');
      }
  
      public static function getUrl($type, $customerId) {
          return self::build($type)->getUrl($customerId);
      }
  
      public static function bounce($type, $customerId) {
          $url = self::getUrl($type, $customerId);
          header("Location: $url");
      }
  }
  
  Redirects::addRedirectType('login', new LoginRedirect);
  Redirects::addRedirectType('register', new RegisterRedirect);
  

  And your redirect code becomes

  $redirectUrl = RedirectFactory::build('register')->getUrl(102);
  header('Location: ' . $redirectUrl);
  

  Or:

  $redirectUrl = RedirectFactory::getUrl('register', 102);
  header('Location: ' . $redirectUrl);
  

  Or even better:

  RedirectFactory::bounce('register', 102);
  

  Of course, at that point, this really isn't a "factory" anymore, and should probably change its name. You probably also don't gain anything from having build be public.

  I have to wonder, though, whether RedirectInterface and its relatives are overengineering. Single-method interfaces and classes like this are generally just a workaround for languages that lack any functional programming capability.

Answer 2

Here is overhauled version of above code:

<?php

//File: RedirectUrlInterface.php
interface RedirectUrlInterface
{
    public function buildUrl();
}

//File: LoginRedirectUrl.php
class LoginRedirectUrl implements RedirectUrlInterface
{
    protected $customerId;
    public function __construct($customerId)
    {
        $this->customerId = $customerId;
    }

    public function getCustomerId()
    {
        return (int) $this->customerId;
    }

    public function buildUrl()
    {
        // do some business logic here to get url
        $url = '/account/id/' . $this->getCustomerId();
        return $url;
    }
}

//File: RegisterRedirectUrl.php
class RegisterRedirectUrl implements RedirectUrlInterface
{
    protected $customerId;
    public function __construct($customerId)
    {
        $this->customerId = $customerId;
    }

    public function getCustomerId()
    {
        return (int) $this->customerId;
    }

    public function buildUrl()
    {
        // do some business logic here to get url
        $url = '/welcome/id/' . $this->getCustomerId();
        return $url;
    }
}

//File: StrategyRedirectUrl
class StrategyRedirectUrl
{
    protected $redirectType;

    public function setType(RedirectUrlInterface $redirectUrlInterface)
    {
        $this->redirectType = $redirectUrlInterface;
        return $this;
    }

    public function getUrl()
    {
        return $this->redirectType->buildUrl();
    }
}



/******************************************/
//Usage:
/******************************************/
// 1. Usage: Login.php (After Login)
class Login
{
    protected $strategyRedirectUrl;
    protected $customerSession;

    public function __construct(
        StrategyRedirectUrl $strategyRedirectUrl,
        CustomerSession $customerSession
    ) {
        $this->strategyRedirectUrl  = $strategyRedirectUrl;
        $this->customerSession      = $customerSession;
    }

    public function afterLogin()
    {
        $customerId = $this->customerSession->getId();
        // Usage example
        $redirectUrl = $this->strategyRedirectUrl
            ->setType(new LoginRedirectUrl($customerId))
            ->getUrl()
        ;
        header('Location: ' . $redirectUrl);
    }
}

// 2. Usage: Register.php (After Registration)
class Register
{
    protected $strategyRedirectUrl;
    protected $customerSession;

    public function __construct(
        StrategyRedirectUrl $strategyRedirectUrl,
        CustomerSession $customerSession
    ) {
        $this->strategyRedirectUrl  = $strategyRedirectUrl;
        $this->customerSession      = $customerSession;
    }

    public function afterRegistration()
    {
        $customerId = $this->customerSession->getId();
        // Usage example
        $redirectUrl = $this->strategyRedirectUrl
            ->setType(new RegisterRedirectUrl($customerId))
            ->getUrl()
        ;
        header('Location: ' . $redirectUrl);
    }
}

NOTES:

• Static factory method has been replaced by dependency injection
• Used Strategy Pattern
• If new redirect type needs to be introduced like GeoIpRedirect, you can similarly implement the RedirectUrlInterface and add your own logic in buildUrl() method

Let me know what you guys think of this.

Answer 3

What about this:

class UrlGenerator {

    public function generateLogin($customerId) {
        return "/login/" + $customerId;
    }

    public function generateRegister($customerId) {
        return "/register/" + $customerId;
    }
}

Don't underestimate the design pattern called function. Only use the more complicated design patterns when it is really necessary.