Is this bad design?

[chadjohnson]

Hi,

 

Suppose I have a control panel web site which allows me to manage multiple web sites. Each site can have multiple groups, and each group can have multiple members. I have the following objects and methods:

 

Site

+--getId()

+--getName()

 

Group

+--getId()

+--getName()

 

Member

+--getId()

+--getName()

 

Would adding the method getGroups() (returning an array of Group objects) to Site and the method getMembers() (returning an array of Member objects) to Group be bad design?

[DarkWater]

How would you be implementing the getMembers() method?  Do you actually store an array of members in the group?

[chadjohnson]

How would you be implementing the getMembers() method?  Do you actually store an array of members in the group?

 

Hi, no, I don't store the array of members in the group--unless I want to cache them to make repeated access faster.

 

The function would query a database for a list of members associated with the group (SELECT member_id FROM group_members WHERE group_id=12).

[DarkWater]

Nooo.  A group object has nothing to do with retrieving members from a database.  Use a data mapper to fill the group object with an array of Member objects possibly.

[chadjohnson]

Something like this?

class MemberGroupMapper {
  public static function populateGroup($groupId) {
    $members = array();

    // Query database for all ids of members in the group.

    foreach ($membersIds as $memberId) {
      $member[] = Member($memberId);
    }

    $group = new Group($groupId);
    $group->setMembers($members);
  }
}

 

Is that what you mean? What is the point in that?

[DarkWater]

Read the tutorial set on OOP, starting with the first one, and hopefully it will clear some things up.

http://www.phpfreaks.com/tutorial/oo-php-part-1-oop-in-full-effect

[chadjohnson]

Thanks, but that does not help. I already understand object-oriented syntax in PHP very well, and I've studied most of the GoF design patterns. The tutorial you linked to does not provide any insight into using or why to use a data mapper to fill another object.

[DarkWater]

Okay then, specifically tutorial 2, the section on coupling and cohesion.  They're pretty important principles.  Since a Group object doesn't necessarily have anything to do with a database, it should not populate itself through one.  =/

[chadjohnson]

Very, very excellent tutorial. Thank you very much.

 

Okay, so it seems it would be better to do something like this:

Classes:

Site
+--getId()
+--getName()

Group
+--getId()
+--getName()

Member
+--getId()
+--getName()

DataMapper
+--$id
+--__construct($id)

GroupDataMapper extends DataMapper
+--__construct($id)
+--findMembers()

 

Usage:

$group = new Group($groupId);
echo $group->getName();

$mapper = new GroupDataMapper($groupId);
$members = $mapper->getMembers();

foreach ($members as $member) {
  echo $member->getName() . "\n";
}

 

What do you think?

[DarkWater]

You wouldn't instantiate a Group object outside of the data mapper in this case.