3
\$\begingroup\$

Please review my class. It uses iconv() (it's probably not the best solution however I haven't found any better alternative to change unknown characters).

<?php

/**
 * Class NiceUrl
 *
 * Creates friendly url from given name
 */
class NiceUrl
{
    /**
     * Whether convert to lower
     *
     * @var bool|null
     */
    private $lower = true;

    /**
     * Rule for removing unwanted characters - regex (PCRE)
     *
     * @var null|string
     */
    private $removeRule = '/[^a-zA-Z0-9]/';

    /**
     * Splitter for words
     *
     * @var null|string
     */
    private $splitter = '-';

    /**
     * Input string encoding
     *
     * @var null|string
     */
    private $encoding = 'UTF-8';


    /**
     * Sets default parameters for all conversions. If null provided it uses
     * defaults set in class properties
     *
     * @param null|string $removeRule @see $removeRule
     * @param null|bool   $lower      @see $lower
     * @param null|string $splitter   @see $splitter
     * @param null|string $encoding   @see $encoding
     */
    function __construct(
        $removeRule = null,
        $lower = null,
        $splitter = null,
        $encoding = null
    ) {
        if (!is_null($removeRule)) {
            $this->removeRule = $removeRule;
        }
        if (!is_null($lower)) {
            $this->lower = $lower;
        }
        if (!is_null($splitter)) {
            $this->splitter = $splitter;
        }
        if (!is_null($encoding)) {
            $this->encoding = $encoding;
        }
    }


    /**
     * Generates url from given name. If other parameters are null, there are
     * used settings given in class constructor or default ones
     *
     * @param string      $name       String to generate url
     * @param null|string $removeRule @see $removeRule
     * @param null|bool   $lower      @see $lower
     * @param null|string $splitter   @see $splitter
     * @param null|string $encoding   @see $encoding
     *
     * @return string Generated url
     */
    public function create(
        $name,
        $removeRule = null,
        $lower = null,
        $splitter = null,
        $encoding = null
    ) {
        if (is_null($removeRule)) {
            $removeRule = $this->removeRule;
        }
        if (is_null($lower)) {
            $lower = $this->lower;
        }
        if (is_null($splitter)) {
            $splitter = $this->splitter;
        }

        if (is_null($encoding)) {
            $encoding = $this->encoding;
        }


        $url = str_replace(
            "'",
            '',
            iconv($encoding, "ASCII//TRANSLIT//IGNORE", trim($name))
        );


        if ($lower) {
            $url = strtolower($url);
        }

        $url = preg_replace($removeRule, $splitter, $url);
        $url = preg_replace(
            '/(' . '\\' . $splitter . '){2,}/',
            $splitter,
            $url
        );

        $url = trim($url, $splitter);

        return $url;
    }

}

I've created also small PHPUnit test class for it:

<?php
/**
 * PhpUnit test for NiceUrl class
 */

require('NiceUrl.php');

class NiceUrlTest extends PHPUnit_Framework_TestCase
{

    public function testBasicPolishUrls()
    {
        $u = new NiceUrl();
        $this->assertEquals('abc', $u->create('ąbc'));
        $this->assertEquals('aa2escolzzcn3', $u->create('ąą2ęśćółżźćń3'));
        $this->assertEquals('abc', $u->create('abc '));
        $this->assertEquals('a-b-c', $u->create('a  b c '));
        $this->assertEquals(
            'aaescolzzcn-aescolzzcn',
            $u->create('ąąęśćółżźćń ĄĘŚĆÓŁŻŹĆŃ')
        );
        $this->assertEquals('a-b-c', $u->create('a|b||c'));
        $this->assertEquals('a-b-c', $u->create('a/b/c'));
    }

    public function testComplexPolishUrls()
    {
        $u = new NiceUrl();
        $this->assertEquals('', $u->create('ąbc', '/[^A-Z]/', false, '-'));
        $this->assertEquals('aaescolzzcn', $u->create('ąąęśćółżźćń'));
        $this->assertEquals('abc-abc', $u->create('abc ABC2', '/[^a-zA-Z]/'));
        $this->assertEquals(
            'abc-ABC',
            $u->create('abc ABC2', '/[^a-zA-Z]/', false)
        );
        $this->assertEquals(
            'a/b/19',
            $u->create('a  b C 019', '/[^a-b1-9]/i', false, '/')
        );
    }

    public function testOtherUrls()
    {
        $u = new NiceUrl('/[^A-Z2-9a-z_]/', false, ',');
        $this->assertEquals('A,O,Uss', $u->create('0Ä1ÖÜß1'));
        $this->assertEquals('OEAEoeae', $u->create('ήϾ'));

    }
}
\$\endgroup\$
1
\$\begingroup\$

Nice Code, just a couple of points:

Constructor and Default Values

If you use the default values in the constructor like this:

function __construct($removeRule = '/[^a-zA-Z0-9]/', $lower = true, 
        $splitter = '-', $encoding = 'UTF-8' ) {
    $this->removeRule = $removeRule;
    $this->lower = $lower;
    $this->splitter = $splitter;
    $this->encoding = $encoding;
}

you can save all the null-checks.

But as you pointed out in the comments, this approach somewhat limits the usability. An alternative to at least remove the duplicate checks is to extract the check to their own method:

private function checkParams($removeRule, $lower, $splitter, $encoding) {
    if (!is_null($removeRule)) {
        $this->removeRule = $removeRule;
    }
    if (!is_null($lower)) {
        $this->lower = $lower;
    }
    if (!is_null($splitter)) {
        $this->splitter = $splitter;
    }
    if (!is_null($encoding)) {
        $this->encoding = $encoding;
    }
}

and then call this method in the constructor and the create method:

$this->checkParams($removeRule, $lower, $splitter, $encoding);

create-Method Parameters

Here, you could also do the same as in the constructor. But I don't think that this is really necessary. You don't need two places to set these values, so I would remove them from create (at a maximum I would add getters and setters for them, so a user could construct a NiceUrl object and then change one of the values later on).

I would assign the value of $name directly at the start of the method to $url, and then only use $url from there on. Or get rid of the $url property and use $name throughout.

create-Method

I would move the trim function on its own line outside the iconv function, as they do different things.

Right now, you allow the url to be empty. Is that something that should be allowed? If not, return a default value at the end in case $url is empty.

Encoding

For me, your approach does not work. ąą2ęśćółżźćń3 for example becomes 2-3. If I use $url = htmlentities($name, ENT_COMPAT, 'UTF-8'); instead of your str_replace/iconv line, I at least get 2-oacute-3. I'm not sure why this is, and I tried searching for a better solution, but couldn't find any.

I don't want your solution to get lost, and since you cannot edit your original code, here is the code that works:

   if (function_exists('iconv')) {
      $oldLocale = setlocale(LC_CTYPE,0);
      setlocale(LC_CTYPE, 'en_US.utf8');
      $url = iconv($encoding, "ASCII//TRANSLIT//IGNORE", $url);
      setlocale(LC_CTYPE, $oldLocale);
   }
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. 1) My conception for constructor is that now you can use null, null, ',' as arguments. So you only change splitter. If I put here defaults as you suggested, it wouldn't work here. For create method 2) I used those arguments just to give you possibility to generate url with other parameters (not those from constructor) so I've decided not to use setters. 3)$nameand $url and trim - I think good point. 4) How have you tested it? For $u = new NiceUrl(); echo $u->create('ąą2ęśćółżźćń3')."<br />"; it gives me aa2escolzzcn3 as result and not 2-3 \$\endgroup\$ – Marcin Nabiałek Aug 20 '14 at 10:23
  • \$\begingroup\$ @MarcinNabiałek 1) good point. 2) I would prefer getters and setters for that, but if you want to use your approach: I would extract the parameter check to a method, so you only have that code once 4) yes, that's how I tested it. It is probably by setup. \$\endgroup\$ – tim Aug 20 '14 at 10:43
  • \$\begingroup\$ @MarcinNabiałek it will still work with null passed in. The default value given in the constructor is used when the passed-in parameter is null ;) \$\endgroup\$ – Pinoniq Aug 20 '14 at 12:37
  • \$\begingroup\$ @tim Could you please test it one more time (I added setting locale for iconv) \$\endgroup\$ – Marcin Nabiałek Aug 20 '14 at 12:39
  • \$\begingroup\$ @Pinoniq Do you mean create method? If yes, that's the point. In constructor you set your default settings (or nothing if you want to use predefined settings). But for each calling of create() method you could pass other settings if you want to (probably it real application it wouldn't be used at all or used very rare) \$\endgroup\$ – Marcin Nabiałek Aug 20 '14 at 12:41
1
\$\begingroup\$
/**
 * Class NiceUrl
 *
 * Creates

Stop here. Classes don't create things, classes are things, with their own properties and behaviour that distinguishes them from other things. Since there's no such "thing" as a "NiceUrl", it shouldn't be a class. What you're trying to write here is a function which takes one string and possibly a set of options, and spits out another one:

function nice_url($name, $options=[]) {
    $defaults = [
        'lower'    => true,
        'remove'   => '/[^a-zA-Z0-9]/',
        'splitter' => '-',
        'encoding' => 'UTF-8'
    ];
    $options += $defaults;

    $url = iconv($options['encoding'], "ASCII//TRANSLIT//IGNORE", trim($name));
    $url = str_replace("'", '', $url);
    if ($options['lower']) {
        $url = strtolower($url);
    }
    if ($options['remove']) {
        $url = preg_replace($options['remove'], $options['splitter'], $url);
    }
    $url = implode($options['splitter'], array_filter(explode($options['splitter'], $url)));
    return $url;
}

....

$url = nice_url('FooBar', ['lower' => false]);

The code itself looks allright to me, I cleaned up a couple of things (e.g. the splitter regex).

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I think that there definitely could be a thing like NiceUrl. But you are right, as it is used here, a class is not really necessary. Also, your answer could benefit from explaining some of the things that you cleaned up (options array, splitter regex, etc). It's always better to explain than to just post improved code. \$\endgroup\$ – tim Aug 24 '14 at 12:41
  • \$\begingroup\$ @tim: the code is rather self-explanatory, but if the OP (or anyone) asks for clarifications, I'll be glad to add them. \$\endgroup\$ – georg Aug 24 '14 at 15:49

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.