Parse a clean slug URL

Question

I have to implement a "user-friendly" URL for an employment website. The URL /jobs{parameters?} may be composed of up to three searching criteria separated by hyphens (i.e. -); each criterion is separated by a tilde (i.e. ~). The parameters string has the following structure: -keywords-employmentTypes-locations. keywords and employmentTypes are optional. location is mandatory unless there are no parameters, the route should handle all these cases:

/jobs

/jobs-canada     /jobs-canada~paris~usa

/jobs-full_time-canada  /jobs-full_time~part_time-canada~paris
/jobs-engineer-canada  /jobs-engineer~c++-canada~paris

/jobs-engineer-full_time-canada  /jobs-engineer~c++-full_time-canada~paris

The following class parses the string parameters and fill all criteria in variables:

class JobUrlBuilder
{
    const DEFAULT_LOCATION = 'france';

    public $keywords;
    public $employmentTypes;
    public $locations;

    public $allEmploymentTypes;

    function __construct($allEmploymentTypes = [])
    {
        $this->setAllEmploymentTypes($allEmploymentTypes);
    }

    /**
     * @param string $parameters
    */
    public function setParameters($parameters)
    {
        $parameters = explode('-', ltrim($parameters, '-'));
        if (empty($parameters[0])) {
            array_shift($parameters);
        }

        if (count($parameters) === 3) {
            $this->keywords  = $parameters[0];
            $this->employmentTypes = $parameters[1];
            $this->locations = $parameters[2];
        } else if (count($parameters) === 2) {
            $areEmploymentTypes = $this->areEmploymentTypes(explode('~', $parameters[0]));

            $this->keywords  = $areEmploymentTypes ? null : $parameters[0];
            $this->employmentTypes = $areEmploymentTypes ? $parameters[0] : null;
            $this->locations = $parameters[1];
        } else if (count($parameters) === 1) {
            $this->keywords  = null;
            $this->employmentTypes = null;
            $this->locations = $parameters[0];
        } else {
            $this->keywords  = null;
            $this->employmentTypes = null;
            $this->locations = self::DEFAULT_LOCATION;
        }

        $this->keywords = $this->keywords ? explode('~', $this->keywords) : [];
        $this->employmentTypes = $this->employmentTypes ? explode('~', $this->employmentTypes) : [];
        $this->locations = $this->locations ? explode('~', $this->locations) : [];

        return $this;
    }

    /**
    * @param array $items
    */
    public function areEmploymentTypes($items)
    {
        $jobTypes = array_map(function ($v)
        {
            return mb_strtolower($v);
        }, $this->allEmploymentTypes);

        return ! array_diff($items, $jobTypes);
    }

    /**
    * @param array $employmentTypes
    */
    public function setAllEmploymentTypes($employmentTypes)
    {
        $this->allEmploymentTypes = $employmentTypes;
    }

    public function hasDefaultLocation()
    {
        return in_array(self::DEFAULT_LOCATION, $this->locations);
    }
}

I don't like this code (spaghetti code, no separation of concerns...). I think there is a better implementation. I'm also open to other URL structures - I know I can simply use input parameters and call it a day, but I should implement a clean URL. I have added unit tests to handle all cases. I think it might help you understand the code.

Answer 1

The greatest problem with this is that you want to make these elements optional without actually marking whether they are present. There are at least two alternatives to this or normal GET parameters that allow for all three to be optional.

Mark whether they are present

Consider a URI that looks like /jobs/keyword/engineer/c++/type/full_time/part_time/location/canada or /jobs-keyword-engineer-c++-type-full_time-part_time-location-canada (if you want everything in the first slot for search engine optimization). You may prefer something like kword, etype, and loc to make those less likely to show up as keywords themselves. I would dislike those as variable names, but the exigencies here may make them better as parameter names. Ideally those would be things that make no sense as keywords.

This is still slug based. It simply has some extra information to help you parse.

Look up everything

You currently have a PHP-based list of valid employment types. Instead, put every valid employment type and location in a database table. Then you can say something like

SELECT parameter_id, parameter_name, parameter_type 
FROM parameters 
WHERE parameter_name IN (?, ?, ?, ?, ?)

where you then pass (for example)

['engineer', 'c++', 'full_time', 'part_time', 'canada']

as the things to select.

I would prefer to store employment types in the database, as they are data to me. Storing them in code is itself a spaghetti code problem in my opinion.

The results will be all the employment types and locations, which you can then easily represent by objects or IDs. Whatever remains will be the keywords, which presumably aren't bounded in the same way that employment types and locations are.

Answer 2

Perhaps Simon is right with the comment:

FWIW I think you should just use input parameters, it's not like anyone is going to type this out.

But if you don't agree with that, then perhaps the simplification below will help.

Possible simplification

One consideration I had was to use a regular expression with named subpatterns- something like this:

preg_match('#-?((?P<keywords>[^-]+)-)?((?P<employmentTypes>[^-]+)-)?(?P<location>.+)#', $parameters, $matches);

Which works well for the case where all three parameters are supplied - e.g. "-engineer~c++-full_time-canada~paris":

$matches =>  Array
(
    [keywords] => engineer~c++
    [employmentTypes] => full_time
    [location] => canada~paris
) //removed numeric keys

So then you can set the properties accordingly:

foreach(['keywords', 'employmentTypes', 'location'] as $key) {
    if ($matches[$key]) {
        $this->$key = $matches[$key];
    }
}

However if either keywords or employment types are omitted, there would still need to be logic to determine which of those is still present... So perhaps it would be better to give more generic names for the first two - like groupA and groupB...

preg_match('#-?((?P<groupA>[^-]+)-)?((?P<groupB>[^-]+)-)?(?P<location>.+)#', $parameters, $matches);

And you would still need the logic to determine if groupA had keywords or employment types... but at least you should likely be able to simplify assignment of the locations parameter.

Simplifying the array_map() call

The call to array_map() in areEmploymentTypes():

 $jobTypes = array_map(function ($v)
    {
        return mb_strtolower($v);
    }, $this->allEmploymentTypes);

Can be simplified by simply passing the function name mb_strtolower as a string literal:

$jobTypes = array_map('mb_strtolower', $allEmploymentTypes);

While there may not be a noticeable improvement in speed, it eliminates the excess closure.