HTTP redirects after login, registration, and logout

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?

-
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.

StackExchange Code Review Q#161208, answer score: 5