1
\$\begingroup\$

I wrote a class to generate URL as I need. (Using http or https and switching off the rewrite rules) That is my first OOP code. It's the right approach, any suggestion?

inc/class.urls.inc.php

    class Url {

    public function __construct() {

        $this->rewrite = SITE_REWRITE;
        $this->domain = SITE_URL;
    }

    public function home() {
        return $this->domain;
    }

    public function page($page) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . $page . '.html'; // https://www.example.com/contact.html
        } else {
            return $this->domain . '/' . $page . '.php'; // https://www.example.com/cantact.php
        }
    }

    public function manufacter($slug) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . $slug; // https://www.example.com/apple
        } else {
            return $this->domain . '/manufacter.php?s=' . $slug; // https://www.example.com/manufacter.php?s=apple
        }
    }

    public function product($slug, $manufacter) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . $manufacter . '/' . $slug; // https://www.example.com/apple/iPhone
        } else {
            return $this->domain . '/product.php?s=' . $slug; // https://www.example.com/product.php?s=iPhone
        }
    }

    public function category($slug) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . 'categories/' . $slug; // https://www.example.com/categories/phone
        } else {
            return $this->domain . '/category.php?s=' . $slug; // https://www.example.com/category.php?s=phone
        }
    }

}

Page

<?php

DEFINE('SITE_NAME','Site Name');
DEFINE('SITE_DOMAINS','www.exemple.com');
DEFINE('SITE_HTTP','https://');
DEFINE('SITE_URL',SITE_HTTP.SITE_DOMAINS);
DEFINE('SITE_REWRITE',1);

include_once 'inc/class.urls.inc.php';

$url = new Url();

echo '<b>Home:</b> '.$url->home() ."<br>";
echo '<b>Page:</b> '.$url->page('contacts') ."<br>";
echo '<b>Category:</b> '.$url->category('ipa') ."<br>";
echo '<b>Product:</b> '.$url->product('punk-ipa','brew-dog') ."<br>";
echo '<b>Manufacter:</b> '.$url->manufacter('brew-dog') ."<br>";
?>

Result

Home: https://www.exemple.com
Page: https://www.exemple.com/contacts.html
Category: https://www.exemple.com/categories/ipa
Product: https://www.exemple.com/brew-dog/punk-ipa
Manufacter: https://www.exemple.com/brew-dog
\$\endgroup\$
2
\$\begingroup\$

You are on the right track, but there is some hidden complexity and repetition:

if ($this->rewrite == 1) {
    // ...
} else {
    // ...
}

The if statement above is peppered all throughout your code. You actually have a need one base class Url and two child classes that specialized in the different routing strategies. First, you'll want the Url class to be abstract, and specify all the methods that return URLs to be abstract as well. The one thing both routing strategies share in common is the domain name, which should be a private field with a protected getter to enforce encapsulation.

Lastly, you see a static method called create which chooses which concrete instance to return based on environmental variables.

abstract class Url {
    private $domain;

    public function __construct($domain) {
        $this->domain = $domain;
    }

    public abstract function home();
    public abstract function page($page);
    public abstract function manufacter($slug);
    public abstract function product($slug, $manufacter);
    public abstract function category($slug);

    protected function getDomain() {
        return $this->domain;
    }

    public static function create(): Url {
        if (SITE_REWRITE) {
            return new SearchEngineFriendlyUrl(SITE_URL);
        }

        return new PhpFileUrl(SITE_URL);
    }
}

Now you need one concrete class for each routing strategy:

class PhpFileUrl extends Url {
    public function home() {
        // ...
    }

    public function page($page) {
        // ...
    }

    public function manufacter($slug) {
        // ...
    }

    public function product($slug, $manufacter) {
        // ...
    }

    public function category($slug) {
        // ...
    }
}

class SearchEngineFriendlyUrl extends Url {
    public function home() {
        // ...
    }

    public function page($page) {
        // ...
    }

    public function manufacter($slug) {
        // ...
    }

    public function product($slug, $manufacter) {
        // ...
    }

    public function category($slug) {
        // ...
    }
}

Once you have this, you no longer need IF statements anywhere:

class PhpFileUrl extends Url {
    public function page($page) {
        return $this->getDomain() . '/' . $page . '.php';
    }
}

class SearchEngineFriendlyUrl extends Url {
    public function page($page) {
        return $this->getDomain() . '/' . $page . '.html';
    }
}

Each class becomes very clean and easy to understand. Furthermore, if your SEO friendly URLs change, you need only change the one class without affecting the other, which reduces your testing effort.

Remember that every branch in your code (think if statement and switchs) is another path to failure, and therefore another path you must test... in each method the branch exists.

Now, using this is just as easy as before, but gives you a more maintainable solution:

$url = Url::create();

echo $url->page('about');
\$\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.