How to avoid this addition to our god object

Question

Our web application allows users to upload files. We have two god objects in our web application:

User object (3000+ lines)
File object (3000+ lines)

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:

Is adding the user object to the file object a bad idea?
If it's a bad idea, what is the main argument for not doing so?
How else can I go about refactoring these objects?

Notes

I know number of lines aren't a true measure, but our other classes are 50-150 lines and rarely used.
I've cut down the common usage to the important bits so apologies for lack of best practices.
I'm already trying to remove static methods and public variables from our code.

Why wouldn't you add an object as a property? a class is a type as any other... why differentiate between an array, an object or a string? — NDM, Nov 06 '13 at 15:45
I agree. It just feels somehow our two god objects have now become one. — paul, Nov 06 '13 at 15:46
the owner is a property of the file, it's just the way it is in your data model. Representing it this way in the code does not make it a god object. In fact, it takes away any functions on the File object that has to check the validity of any ownerID being set. — NDM, Nov 06 '13 at 15:47
Ok thanks, do you have any advice on how to reduce the size of these classes then? For example, where does the static method File::getByID belong? — paul, Nov 06 '13 at 15:54
Ive updated my answer to add additional info about reducing the class's size. — NDM, Nov 06 '13 at 16:33

NDM · Accepted Answer · 2013-11-06T16:33:19.267

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.

Would the FileMeta object be passed in as a constructor argument? — paul, Nov 06 '13 at 16:37
I wouldn't necessarily do that, because it can easily be constructed with the data inside the class, and thus can be constructed in a getter. this will prevent a bloated constructor. but dependency injection fans will probably shoot me for saying this. (it could make testing a little harder) — NDM, Nov 07 '13 at 10:41

How to avoid this addition to our god object

1 Answers1