Problem
I have the following code to generate an URL based on alias and args provided
/**
* Get the url by alias.
*
* @param string $alias
* @param array $args
*
* @return mixed
*/
public function url($alias, array $args = [])
{
if (empty($this->map[$alias])) {
return null;
}
$segments = explode('/', $this->map[$alias]);
$url = [];
foreach ($segments as $segment) {
if (false !== strpos($segment, ':')) {
$arg = current($args);
if (null === $arg && false === strpos($segment, '?')) {
return null;
} elseif (null !== $arg) {
$url[] = $arg;
}
continue;
}
$url[] = $segment;
}
$aliases = array_keys($this->patterns);
$patterns = array_values($this->patterns);
$pattern = str_replace($aliases, $patterns, $this->map[$alias]);
if (preg_match('~^'.$pattern.'$~', $url = implode('/', $url))) {
return $url;
}
}
And the $this->map and $this->patterns are as follows (
[map] => Array
(
[home] => /
[about] => /about
[posts] => /posts/:slug?
)
[patterns] => Array
(
[:number] => ?([0-9]+)
[:slug] => ?([^/]+)
)
Usage
$class->url('home'); //outputs '/'
$class->url('about'); //outputs '/about'
$class->url('posts', ['winter-is-comming']); //outputs '/posts/winter-is-comming'
I would like to know what could be the areas of improvement here? For example, pass variables by name not sequence, more patterns, etc.
Solution
Using the syntax you described and the map and patterns properties, I have attempted to create function, which replaces arguments according to names instead of sequence. Before showing my solution I would like to review your code with a focus on the differences between the two solutions.
The first thing that puzzles me, is the order of your condition in your if statements. To me it seems counter-logical that the expected value is written before the source of the value. I know this is a minor thing as some coding standards promote this. You could also use a ‘early return’ in your loop by inverting the logic, so that the segment is added and the loop continue if the value is fixed. This helps keeping the indentation level down, which improves readability.
Next I have found two possible bugs. The first being how you fetch the value of the current argument. Using current() without calling next() later in the loop will leave you stuck at the same value as the array index pointer is never advanced. Therefore I would add a call to next() at the end of your loop.
The second bug is your regular expressions. When I use them the preceding question mark (ex. ?([^/]+)) gives me a regular expression compile error. I also cannot see the reason for them, so they have been removed.
The solution I have come up with is based on your approach and looks like the following:
/**
* Generates an url from a route name.
*
* @param string $name An unique string.
* @param array $parameters An associative array of URL parameters. The array index must
* match a segment in the URL.
* @param array $query An associative array of key/value pairs, which should be used
* inside a HTTP URL query string.
*
* @return string
*/
function url($name, array $parameters = [], array $query = [])
{
if(!array_key_exists($name, $this->map))
{
throw new LogicException("Unknown route: {$name}");
}
if(strpos($this->map[$name], '/:') === false)
{
return $this->map[$name];
}
$resolved = [];
$segments = explode('/', ltrim($this->map[$name], '/'));
foreach($segments as $key)
{
if(strpos($key, ':') === false)
{
$resolved[] = $key;
continue;
}
$length = strlen($key) - 1;
$optional = false;
if($key[$length] == '?')
{
$key = substr($key, 0, $length);
$optional = true;
}
if(!array_key_exists($key, $parameters))
{
/*
* If the argument was optional we can break out of the loop.
*/
if($optional)
{
break;
}
throw new LogicException("Missing URL parameter: {$key} for route: {$name}");
}
/*
* If you want to check if the parameter type match you can do the following. By
* checking for each argument, you can specify which argument is the source of
* the error easier.
*
* This is also a performance hit if you are calling this code several times each request
* with URLs that have many arguments. You can explore alternatives, but remember premature
* optimization is almost never good.
*/
if(!preg_match('~^' . $this->patterns[$key] . '$~i', $parameters[$key]))
{
throw new LogicException("Invalid type of URL parameter: {$key} in route: {$name}");
}
$resolved[] = $parameters[$key];
}
$haystack = '/' . implode('/', $resolved);
/*
* This is totally optional, but I added this
* as I know from experience it can be very useful.
*/
if($query)
{
$haystack .= '?' . http_build_query($query);
}
return $haystack;
}
I have added the possibility to append a HTTP query string to the generated URL. The mapand patterns class properties are left unchanged. I hope you can see the similarities between the two solutions, as it was my goal not to change it too much.
As said in the code comments, checking type with regular expressions can be a performance issue. I started down the same path, but soon removed the feature as the URL generation solution I have built is run several times and therefore made a noticeable performance hit.
You also asked for additional patterns. I have found that only 4 are really required for the general use case. I have defined them as follows:
/*
* This is from my personal library.
*/
private $types = [
'*' => '[^/]+', // Everything except a forward slash
'int' => '[d]+', // Only numbers
'char' => '[w]+', // Only alphabetic characters
'alnum' => '[wd]+', // Alphabetic characters and numbers, so special characters
];
I hope this can help, feel free to ask questions and happy coding!