2
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Seems like this is less a code review than a hypothetical "how would you refactor this?" question. Do you want to show how you refactored this and get that reviewed? \$\endgroup\$ – Mike Brant Apr 19 '17 at 16:34
  • \$\begingroup\$ This is a show case of how I have done. And willing to know how it can be better. \$\endgroup\$ – MagePsycho Apr 19 '17 at 17:20
  • 1
    \$\begingroup\$ This all seems overly complicated just to perform redirect. Maybe that is because your have excluded pertinent parts of your code? \$\endgroup\$ – Mike Brant Apr 19 '17 at 17:46
4
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ How would you handle the object factory in case of constructor based dependency injection? \$\endgroup\$ – MagePsycho Apr 19 '17 at 17:23
  • 2
    \$\begingroup\$ @MagePsycho: I wouldn't -- at least not much more than is already provided for. You can already inject any implementation of RedirectInterface you want, constructed however you want. If you want independent scopes for redirect types, or want to be able to construct a RedirectFactory, you can make everything non-static. If you want to actually build objects, you can make $urlBuilders contain functions instead, and return the result of calling the function. But keep in mind, you're calculating a freaking URL. Don't complicate it any more than necessary. \$\endgroup\$ – cHao Apr 19 '17 at 17:45
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I like the Class name 'UrlGenerator'. Since the generating login & register url is quite complex should go in a individual classes extending from some Abstract class which holds some common properties/methods. \$\endgroup\$ – MagePsycho Apr 22 '17 at 12:01
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.