Jump to content

Is this bad design?


chadjohnson

Recommended Posts

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?

Link to comment
Share on other sites

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).

Link to comment
Share on other sites

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?

Link to comment
Share on other sites

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?

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.