3

Say I have a many to many relationship in the database like this:

CREATE TABLE Person (ID int, name varchar(100), dateofbirth datetime, Gender char(1), primary key (ID))
CREATE TABLE Sport (ID int, description varchar(30), primary key (ID))
CREATE TABLE PersonCanPlaySport (Person ID int references Person(ID), SportID int references Sport(ID), PRIMARY KEY (SportID, PersonID))

The tables show what sports a person can participate in. Now see the class below:

public class Person
{
   public List<Sport> Sports;
   public int FitnessLevel;
   public datetime DateOfBirth;
   public string Gender; //M or F

   public Person()
   {
     Sports = new List<Sport>();
   }

   //allSports contains all the sports from the database i.e. allSports contains about 100 sports.  AddSports determines what sports (out of the 100) that the user can participate in based on: Gender, age, Fitness Level etc
   public void AddSports(List<Sports> allSports)
   {
      Sports.Clear();
      ((List<Sport>)Sports).AddRange(allSports
      .Where(sport => sport.CanPlay(this)));
   }

}

There appears to be an Association relationship and a Composition relationship between Person and Sports. Here is why:

1) Person creates an instance of a list of sports in the constructor so controls the lifecycle of Sports 2) Person uses a List of sports in AddSports, which I believe makes it Association

Is this a code smell? What kind of relationship exists between the two objects? How would this be represented on an Object Diagram?

Update

Following on from the helpful answer and comments from @Neil; I believe he is suggesting doing this:

//Application Service Layer
List<Sport> sports = Repository.GetAllOffers();
foreach sport As Sport in sports
{
   person.AddSports(sport);
}

//Domain class
public class Person
{
   public List<Sport> Sports;
   public int FitnessLevel;
   public datetime DateOfBirth;
   public string Gender; //M or F

   public Person()
   {
     Sports = new List<Sport>();
   }

   public void AddSport(Sport sport)
   {
      if(sport.CanPlay(this))
      {
         Sports.Add(sport);
      }
   }

}

I am not sure if I like the idea that the Application Service layer has to loop through the Sports.

  • Having a collection does not necessarily mean that Person controls the lifecycle of the Sport elements in that collection. (It does, however, control/own the lifecycle of the collection object itself — but that is a very low level relationship: merely an artifact of (collections in) objects.) – Erik Eidt Nov 21 '17 at 19:33
  • @Erik Eidt, the collection is populated in the constructor meaning the collection is created and destroyed by person making it a composition relationship. Isn't that right? – w0051977 Nov 21 '17 at 19:43
  • no, for these purposes, you have to distinguish between the collection object itself, and the elements in the collection. On the one hand, if the collection object is only referenced by the Person object, destroying (loosing) the Person object will loose the collection object. On the other hand, loosing the collection will not necessarily loose the elements in the collection, especially if they are managed elsewhere, such as having the persistence holding those relationships and entities, so loosing in-memory object doesn't necessarily affect the persistence. – Erik Eidt Nov 21 '17 at 19:56
  • Firstly: not a good design. What if I called AddSports with sports that the person could not play or take part in? But about your question of composition and association: even if you call this composition, the list of sports I passed in could have existed before Person object came into existence and may even exist after Person is garbage collected. Now you may say "ok so then its association"--maybe but maybe not. Depends on what you do with it. – CodingYoshi Nov 22 '17 at 4:08
  • @CodingYoshi, can you post some code in an answer? Are you saying that AddSports should not accept a collection? – w0051977 Nov 22 '17 at 6:11
3

The database relationship between Person and Sport as I understand it is many-to-many, with a intermediate table PersonCanPlaySport that connects the two. This means a Person can be associated with many Sports and a Sport can be associated with many Persons.

Now that we understand the database relationship, the relationship used in the code should be no different. Potentially a Person instance would have a list of Sport and a Sport instance would have a list of Person.

I think therefore it would be incorrect to put the logic of whether or not a Person should have a specific Sport directly inside Person, or vice versa. Consider the use of a PersonSportDao object which given a Person, can return all Sports and given a Sport can return all Persons. The caller to AddSports would make the call to PersonSportDao, or better still, Person instance itself loads its Sports when asked for it, calling PersonSportDao with this.

If you wish to check details regarding age or gender, you could create another class PersonSportManager. In other words, Person isn't owner of Sport and so it should see none of the logic just as Sport isn't owner of Person and so it shouldn't see any of the logic of Person.

Had Person to Sport been a one-to-many relationship, I would have argued otherwise, but I think the code should reflect the symmetry of the relationship on the database.

Edit: Answering your question:

Is this a code smell? What kind of relationship exists between the two objects? How would this be represented on an Object Diagram?

A composition relationship would imply that Sports make up Person which is like a one-to-many relationship between Person and Sport. In your case that isn't true, being many-to-many.

The relationship is too complicated to simply draw a line between the two making it necessary to create a middle class, and then associating Person and Sport both to that middle class (such as PersonSportManager).

Hope that answers your question!

  • Adding an intermediary class is one way of resolving the impedance mismatch. However, instead I have decided to: 1) Add a list of Sports to Person and 2) Not add a List of persons in Sport. This makes the object relationship 0:M. I have edited the question by adding code to the AddSports method. Could you tell me what you think now? – w0051977 Nov 21 '17 at 9:52
  • For the reference to an intermediary class +1. It is one way of approaching it. – w0051977 Nov 21 '17 at 9:53
  • @w0051977 Your way would work too, but you're still tightly coupling Person and Sport this way. If you tell me Sport doesn't have a list of Persons, then you could eliminate the necessity for Sport to know about Person entirely, but it means your method CanPlay needs to be added to Person with a Sport instance passed to it. So long as Sport doesn't need to know about Person, you'll have no conflict. Otherwise, I would recommend my approach. – Neil Nov 21 '17 at 10:59
  • Thank you. So what you are saying is - use my approach is Sport does not need to know about person and use your approach if it does? – w0051977 Nov 21 '17 at 11:12
  • @w0051977 Yes, essentially. If they need to know about each other, a third class should be added to mediate. Otherwise, your approach is fine. – Neil Nov 21 '17 at 11:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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