Our web application allows users to upload files. We have two god objects in our web application:
Common usage at the moment is:
$file = File::getByID($_GET['file_id']);
$user = User::getByID($file->ownerID);
echo $file->title;
echo $user->name;
We are currently discussing adding the user object to the file object as another public variable. Because everywhere we use the file object, we need the associated user object.
So usage will be:
$file = File::getByID($_GET['file_id']);
echo $file->title;
echo $file->user->name;
We are trying to refactor these god objects, and adding the user object to the file object feels like we are making the problem worse. However I don't really have a good argument to disagree with the proposed change.
It would be great to get some help with the following questions:
Notes
I would definitely add the User
object as a property of the File
class, since this is the actual representation of the data model behind it.
It will also give the code more clarity, and will produce more concise usage code of the File
object.
This will also mean the File
class does not have to do any checking for validity if an owner ID is set: if an object is passed, it can assume it is valid (or it should not have been able to be constructed).
class File
{
protected $owner;
/**
* @var User $owner mandatory
*/
public function __construct(User $owner)
{
$this->setOwner($owner);
}
public function setOwner(User $owner)
{
return $this->owner;
}
public function getOwner()
{
return $this->owner;
}
}
It is now clear that the File
has a mandatory property owner
, which is a User
.
I would also recommend using accessors instead of public properties. This will be more future proof, for example it will also allow you to lazy load the user if you please to do so later on.
class File
{
protected $ownerId;
protected $owner;
public function getOwner()
{
if (!$this->owner && $this->ownerId) {
$this->owner = User::getById($this->ownerId);
}
return $this->owner;
}
}
This implies that your data layer fills the object accordingly, and would also require an adapted constructor, with the loss of automatic Type checking (only the constructor, still has type check in the setter):
/**
* @var $owner int|User the user object or it's ID
*/
public function __construct($owner)
{
if (is_object($owner)) $this->setOwner($owner);
else $this->ownerId = $owner;
}
As for the question of where your static User::getById()
should go: you should separate the data operations (retrieval and persisting of objects) in a separate layer.
class UserStore
{
public function getById($id)
{
// code to retrieve data from where ever, probably database
// or maybe check if the user was already instantiated and registered somewhere...
$user = new User($data['name'], $data['email']/*, ....*/);
return $user;
}
}
For this last one, I would really not recommend building your own library (called an ORM), many great implementations exist. Take a look at Doctrine ORM.
Lastly I would say that reducing the size of a class should not be a goal in itself. you could however reduce it's complexity by extracting logic that is not inherent to the object itself, as is the case with the owner property.
Another example might be the File's path: maybe the path is stored relatively, and you have functions to convert that to an absolute path, or to get the extension or filename. This functionality can be extracted in a seperate FileMeta
class or whatever you want to call is, maybe even a File
class in a different namespace: FileSystem\File
.