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;
1 Answer 1
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?
-
\$\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 thesession
object. However, they did not really relate to sessions. That's whereUtilityClass
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 ofText::shortenTo()
, but couldn't one argue that statics are bad? \$\endgroup\$kasimir– kasimir2014年09月20日 18:05:53 +00:00Commented Sep 20, 2014 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– Thomas Maurstad Larsson2014年09月21日 16:15:27 +00:00Commented Sep 21, 2014 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– kasimir2014年09月21日 17:34:19 +00:00Commented Sep 21, 2014 at 17:34
__get
through yourUtilityClass
or trait. That's just wrong on so many levels. Also: pre-declare properties, always specify the access modifiers and KISS: TheSession
object is where you store session data, why wrap it in aUtilityClass
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\$Session
object is wrapped into theUtilityClass
container, are you referring to theutils
object inSessionClass
? Furthermore, how would you 'use'UtilityClass
? \$\endgroup\$UtilityClass
, because from the code you show here, it's an implementation of a magic getter, which I dislike.Session
isn't wrapped inutils
, but the session object seems to be a data container, why then wrap the data that the session object contains in autils
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 ofutils
? What is the added value of it, and what is the session object doing? \$\endgroup\$UtilityClass
via thesession
object, thus making it available everywhere whilst only instantiating it once. That's the main thought behind it. \$\endgroup\$