0
\$\begingroup\$

Consider this setup:

class UtilityClass
{
    public function formatNumber($input) {
        return number_format($input, 0, '', ' ');
    }

    public function trimText($input) {
        return substr(trim($input), 0, 100);
    }

    // bunch of other methods
}

class SessionClass
{
    public $utils;
    private $permissions = array();

    function __construct() {
        $this->utils = new UtilityClass();
        $this->setPermissions();
    }

    public function getPermissions($type) {
        return $this->permissions[$type];
    }

    public function getSettings() {
        // do stuff
    }

    private function setPermissions() {
        // do stuff to set $this->permissions
    }

    // bunch of other methods
}

class BaseClass
{
    public $session;

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

    // bunch of methods
}

class ExtendedClass extends BaseClass
{
    // bunch of methods
}

$session = new SessionClass();
$extClass = new ExtendedClass($session);

When I want to use the formatNumber method of UtilityClass in my ExtendedClass, I can do it from within ExtendedClass like so:

$this->session->utils->formatNumber('some input');

...because the $session object, containing the $utils object, was injected into the $extClass object. However, if I use the formatNumber method from UtilityClass very often in ExtendedClass, it means quite a lot of (long) calls like written above. I could shorten it by using 'traits', changing:

trait Utilities
{
    function formatNumber($input) {
        // do something and return
    }
}

...and:

class ExtendedClass extends BaseClass
{
    use Utilities;

   // bunch of methods
}

I could then do the same call like this:

$this->formatNumber('some input');

I could put use Utilities in BaseClass so all extended classes could use it. But I would have to put an extra use Utilities in SessionClass, because that needs it as well. So then I end up with a $session object and an $extClass object which both contain the methods from the Utilities trait, and one is injected into the other. That seems wrong.

Another solution is 'aliasing' the formatNumber method from the UtilityClass class. To that end, I could change BaseClass:

class BaseClass
{
    function __construct($session) {
        $this->session = $session;
    }

    function get($input) {
        return $this->session->utils->formatNumber($input);
    }

    // more methods
}

This also seems not quite right. Is there a better solution? What would give the best performance/least redundancy?

Could PHP 5.6 offer the best solution? Using namespaces: Aliasing/Importing

Using use you can now 'alias' a function like this:

// aliasing a function (PHP 5.6+)
use function My\Full\functionName as func;
\$\endgroup\$
  • 1
    \$\begingroup\$ Basically, you're implementing a magic __get through your UtilityClass or trait. That's just wrong on so many levels. Also: pre-declare properties, always specify the access modifiers and KISS: The Session object is where you store session data, why wrap it in a UtilityClass container? That's just adding pointless overhead. Plus: CR focuses on code that you've already written, it seems that you've posted a basic design layout, and are asking us for a concrete (and if possible) better implementation. \$\endgroup\$ – Elias Van Ootegem Sep 19 '14 at 11:57
  • \$\begingroup\$ This is code I've already written, only I've omitted the 'bunch of methods' mentioned as well as the access modifiers and changed the names of classes/variables. My question isn't about those; I've tried to show just enough to explain my question. I do not see how the Session object is wrapped into the UtilityClass container, are you referring to the utils object in SessionClass? Furthermore, how would you 'use' UtilityClass? \$\endgroup\$ – kasimir Sep 19 '14 at 12:14
  • 1
    \$\begingroup\$ First: I wouldn't use UtilityClass, because from the code you show here, it's an implementation of a magic getter, which I dislike. Session isn't wrapped in utils, but the session object seems to be a data container, why then wrap the data that the session object contains in a utils wrapper? Why not store it in the session object directly? Add some more code, though, because this doesn't tell us enough: What's the point of utils? What is the added value of it, and what is the session object doing? \$\endgroup\$ – Elias Van Ootegem Sep 19 '14 at 12:24
  • \$\begingroup\$ I've expanded my example a bit to make clear how the classes and methods are used. The current setup allows me to use the methods from the UtilityClass via the session object, thus making it available everywhere whilst only instantiating it once. That's the main thought behind it. \$\endgroup\$ – kasimir Sep 19 '14 at 12:54
  • \$\begingroup\$ That long call you've shown is a serious code smell, because it breaks Law of Demeter and ISP. Can't tell what's wrong without context (concrete code). Theoretically there might be single instance dependency injected in both session and base/derived class (that depends on session), but it's not likely with utility class. Either session should operate on already "utilised" data or "utilise" passed data itself. \$\endgroup\$ – shudder Aug 16 '15 at 4:19
3
\$\begingroup\$

I'm not a fan of the UtilityClass. Here's some examples:

  • It breaks the single responsibility principle as it is not focused on solving a specific problem.
  • UtilityClass:trimText() conveys too little context about what it does, and the result of calling the method is unexpected. UtilityClass::trimText() would better suit the problem with signature like UtilityClass:shortenTextTo($text, $length);
  • UtilityClass::formatNumber() is not specific enough to explain clearly what it does. How does it format the number?

UtilityClass:trimText() seems like a good fit for a static helper. The problem you are trying to solve looks visual, and you should try to convey that. Look at the following:

class Text {
    public static function shortenTo($text, $length) {
        return substr(trim($text), 0, $length);
    }
}

// Html
<article>
    <h1>My awesome blogpost!</h1>
    <p><?= Text::shortenTo($text, 100); ?></p>
</article>

Now you have a utility for shortening a text to the wanted length.


Your session class also deals with the wrong problem. A session class would represent a Session (i.e a value object) or handle actual session management, but it wouldn't set permissions. An Authenticator class could use a SessionManager to assign specific permissions TO a session, but Sessions should have no understanding of what permissions are.

Hope that gives you something to work on. Your code is not horrible, but it seems to not focus on solving the right problems in the right places. The point of object-orientation is to represent code in realistic, understandable models, and your code does not do that at the moment. Focus on what a Session represents. What actions should you be able to do with a Session object?

\$\endgroup\$
  • \$\begingroup\$ This gives me some ideas on reworking, thanks. SessionClass contained a few methods that were there because of convenience; because all classes could then call them through the session object. However, they did not really relate to sessions. That's where UtilityClass came in. I then started to wonder how to implement this (ie. make it available to all classes). Now I see I should split up some more to create better defined objects. Also, I like the look of Text::shortenTo(), but couldn't one argue that statics are bad? \$\endgroup\$ – kasimir Sep 20 '14 at 18:05
  • \$\begingroup\$ Yes, static code is generally not desirable. However, note that I specifically mentioned visual problems. Since you will most likely never use this for data directly, using a static helper isn't as bad. The "right way" to do this could be to write a real Text object, and having a static factory method or a factory class, but that seems overkill due to how simple the problem is. Good luck. :) \$\endgroup\$ – Thomas Maurstad Larsson Sep 21 '14 at 16:15
  • \$\begingroup\$ Thanks for your reply. The amount of possible solutions is kind of bewildering, especially since you can argue for or against every one of them. Great flexibility but too convoluted, minimal but tightly coupled... However, 'solving the right problem in the right place' is a good compass, I have tended to focus on convenience and keeping the number of objects (and instantiation of them) to a minimum. In that way, this answer is helpful. Even so, it does not really answer the initial question, which was basically to shorten calls like $this->session->utils->trimText(). \$\endgroup\$ – kasimir Sep 21 '14 at 17:34

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.