current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Say you have a number of similar objects (e.g. instances of the class Person). Each of these instances can belong to a number of Groups, also represented by a class. To stick with the examples of people, one person could be a member of the group Employees and SoccerTeam and Family or something.

How would one model this scenario in object oriented design, so that when you remove a person from a group, that change is also being reflected in the persons' list of groups.

My approach would be something like this:

import sets

class Person(object):
  def __init__(self, name):
    self.name = name
    self.groups = sets.Set()

  def register(self, group):
    self.groups.add(group)

  def unregister(self, group):
    self.groups.remove(group)

  def getGroups(self):
    return self.groups

class Group(object):
  def __init__(self, name):
    self.name = name
    self.members = sets.Set()

  def add(self, person):
    self.members.add(person)
    person.register(self)

  def remove(self, person):
    self.members.remove(person)
    person.unregister(self)

  def getMembers(self):
    return self.members

peter = Person("Peter")
family = Group("PetersFamily")

family.add(peter)
# peter is now member of PetersFamily
family.remove(peter)
# peter has been removed from PetersFamily

My gut feeling says this isn't very nice, as calling Person.register from outside a Group instance would break things. What is the right and elegant way to do this?

share|improve this question

closed as off-topic by Vogel612, Mat's Mug, Kid Diamond, RubberDuck, Alex L Sep 20 '14 at 19:21

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Vogel612, Mat's Mug, Kid Diamond, Alex L
If this question can be reworded to fit the rules in the help center, please edit the question.
    
Is it possible to make register() and unregister() acceseable only from Group class? For example, in Java you could place both classes in the same package and use package visibility for the methods register() and unregister(). –  Leonid Semyonov Sep 20 '14 at 15:04
    
@LeonidSemyonov I don't think python has that feature –  mariosangiorgio Sep 20 '14 at 15:29
    
@karlson is this real code or sample code? Real code is on topic, but sample code would not be. Also your last line "What is the right and elegant way to do this?" makes it sound like you might not be looking for a code review, but rather asking a design question. –  bazola Sep 20 '14 at 15:51
    
I have deliberately stripped the example down to a working minimal example, as I thought that to be the usual procedure. I've also posted this question on Stackoverflow in the first place but was downvoted and referred to here. Possibly Programmers is the right place for this kind of problem in the end, but I won't repost my question there, as @mariosangiorgio has already answered it to my satisfaction. thank you –  karlson Sep 22 '14 at 11:38

3 Answers 3

up vote 4 down vote accepted

You have that issue because you are storing that information about the groups each person belongs to in two different places and don't have a way to ensure that the operations you do stay consistent.

A possible solution would be to modify the implementation of Person.register and Group.add to ensure that they always produce a consistent state. You can do it by adding is_in_group(group) to Person and contains(person) to Group.

With that methods, you could redefine register and add in the following way:

def register(self, group):
    if(not group.contains(self)):
        self.groups.add(group)
        group.add(self)

def add(self, person):
    if(not person.is_in(self)):
        self.members.add(person)
        person.register(self)

It would solve your problem but it does not look quite right because of the extremely strong coupling between Person and Group. You should try to find a better way to decouple them. An idea cold be removing the need of keeping a list of groups in Person and a list of people in Group. You could do it by introducing a third class, let's call it Subscriptions, that will contain a list of subscriptions. An easy way to do that could be a (person, group) pair. That class will then offer all the methods to edit subscriptions (with a subscribe(person, group) and an unsubscribe(person, group) as well as methods to query them, like get_groups(person) or get_members(group).

For other decoupling ideas you can have a look at this Q&A on StackOverflow

share|improve this answer
up vote 2 down vote

I like mariosangiorgio's answer better, but here is another way.

The problem stems from a function that calls a function that calls that same function again. You can add another function that doesn't make that calling cycle:

class Person(object):
  def __init__(self, name):
    self.name = name
    self.groups = sets.Set()

  def register(self, group):
    self.groups.add(group)
    group.notifyRegister( self )

  def unregister(self, group):
    self.groups.remove(group)
    group.notifyRemove( self )

  def notifyRegister( self, group ):
    self.groups.add( group )

  def notifyRemove( self, group ):
    self.groups.remove( group )

class Group(object):
  def __init__(self, name):
    self.name = name
    self.members = sets.Set()

  def add(self, person):
    self.members.add(person)
    person.notifyRegister( self )

  def remove(self, person):
    self.members.remove(person)
    person.notifyRemove( self )

  def notifyRegister( self, person ):
    self.members.add( person )

  def notifyRemove( self, person ):
    self.memebers.remove( person )
share|improve this answer
up vote 2 down vote

Use "factory" class to create instances of Person and Group classes and hold shared collection of relations between persons and groups.

For example in Java:

"Factory" class:

public class Society {
    private final Collection<Pair<Person,Group>> relations;

    public Society() {
        relations = new HashSet<Pair<Person,Group>>();
    }

    public Person newPerson() {
        Person person = new Person(relations);
        return person;
    }

    public Group newGroup() {
        Group group = new Group(relations);
        return group;
    }

}

And your classes:

public class Person {
    private final Collection<Pair<Person,Group>> relations;

    Person(Collection<Pair<Person,Group>> relations) {
        this.relations = relations;
    }

    public void register(Group group) {
        relations.add(new Pair<Person,Group>(this, group));
    }

    public void unregister(Group group) {
        relations.remove(new Pair<Person,Group>(this, group));
    }

    public Boolean isIn(Group group) {
        return relations.contains(new Pair<Person,Group>(this, group));
    }

    // !!! override equals(), hashCode()

}

public class Group {
    private final Collection<Pair<Person,Group>> relations;

    Group (Collection<Pair<Person,Group>> relations) {
        this.relations = relations;
    }

    public void add(Person person) {
        relations.add(new Pair<Person,Group>(person, this));
    }

    public void remove(Person person) {
        relations.remove(new Pair<Person,Group>(person, this));
    }

    public Boolean contains(Person person) {
        return relations.contains(new Pair<Person,Group>(person, this));
    }

    // !!! override equals(), hashCode()

}

Using:

Society society = new Society();    
Person person = society.newPerson();
Group group = society.newGroup();
person.register(group);
group.remove(person);
System.out.println(person.isIn(group)); // prints False

But I think it's very redundant. And this way it's harder to subclass Person and Group classes and instantiate them.

share|improve this answer
    
This is quite a clunky way to accomplish this, because both the person and the group are required to have relations and both are resposible to keep them clean. It should be possible to have the relations managed in a separate class, possibly even in Society –  Vogel612 Sep 20 '14 at 16:48
    
@Vogel612 but then there'll be possibility of calling register(), unregister(), add(), remove() methods outside of Person and Group classes, that would break things as @karlson said. –  Leonid Semyonov Sep 20 '14 at 17:03
    
That assumption is incorrect... –  Vogel612 Sep 20 '14 at 17:53

Not the answer you're looking for? Browse other questions tagged or ask your own question.