Instance variable or inheritance?

[Junilu Lacar]

Paul Clapham wrote:But I'm just saying that the data structure you end up with isn't a tree.

In a strict sense, you're right. The data structure taken as a whole will not be a Tree but looking at it from one dimension or another, it can be filtered to look like and be treated like one.

[Sangel Kapoor]

Junilu Lacar wrote:

Paul Clapham wrote:but I certainly wouldn't call them "elegant".

One of my criteria of elegance is to be able to distill and separate complex and difficult to manage ideas into simpler and more manageable ideas in a clear and straightforward way. Separation of concerns helps get you there. The idea that Marriage is when Two become One is not just a romantic concept.

and that is what I did by using HashMaps, if you see into memory, it is a Graph, just representation is elegant. All what you have said can all be done with my implementation. GRAND-FATHER and their Ancestors doen not require new Relationship Classes at all. It could be inferred too in my implementation. :-)
Even Grandfather could be inferred but it is used since it was required to use it.

[Junilu Lacar]

I'd still be interested in seeing the test cases you have.

[Sangel Kapoor]

@RunWith(MockitoJUnitRunner.class)
public class HashBasedFamilyTreeTest {

    public static final String SPACE = " ";
    private FamilyTree familyTree;
    private Person malePerson;
    private Person femalePerson;

    @Mock
    private Map<Relationship, List<Person>> map;

    @Mock
    private Relationship siblings;

    @Before
    public void setup() {
        familyTree = hashBasedFamilyTree(map);
        malePerson = randomMalePerson();
        femalePerson = randomFemalePerson();

        when(siblings.isLegitimate(any(), any())).thenReturn(true);
        when(map.get(siblings)).thenReturn(new ArrayList<Person>());
    }

    @Test
    public void shouldReturnFalseOnAddingIllegitimateRelationshipToFamilyTree() {
        when(siblings.isLegitimate(any(), any())).thenReturn(false);

        boolean result = familyTree.addRelationship(malePerson, siblings, femalePerson);

        assertFalse(result);
    }

    @Test
    public void shouldNotAddIllegtimateRelationshipToFamilyTree() {
        when(siblings.isLegitimate(any(), any())).thenReturn(false);

        familyTree.addRelationship(malePerson, siblings, femalePerson);

        List<Person> siblings = familyTree.getRelationship(this.siblings);

        assertThat(siblings, is(Collections.emptyList()));

    }

    @Test
    public void shouldReturnTrueIfLegitimateRelationshipIsAddedToFamilyTree() {
        boolean result = familyTree.addRelationship(malePerson, siblings, femalePerson);

        assertTrue(result);
    }

    @Test
    public void shouldAddNewLegitimateRelationshipToFamilyTree() {
        familyTree.addRelationship(malePerson, siblings, femalePerson);
        List<Person> spousesAfterFirstAddition = familyTree.getRelationship(siblings);

        assertThat(spousesAfterFirstAddition.size(), is(1));
        assertTrue(spousesAfterFirstAddition.get(0).equals(femalePerson));

        familyTree.addRelationship(malePerson, siblings, femalePerson);
        List<Person> spousesAfterSecondAddition = familyTree.getRelationship(siblings);

        assertThat(spousesAfterSecondAddition.size(), is(2));
        assertTrue(spousesAfterSecondAddition.get(0).equals(femalePerson));

    }

    @Test
    public void shouldReturnEmptyListIfRelationshipIsNull(){
       List<Person> persons = familyTree.getRelationship(null);
        assertThat(persons,is(Collections.<Person>emptyList()));
    }
    @Test
    public void shouldReturnEmptyListIfRelationshipIsNotPresent(){
        List<Person> persons = familyTree.getRelationship(null);
        assertThat(persons,is(Collections.<Person>emptyList()));
    }


    @Test
    public void shouldReturnClonedListOfRelativesRatherThanOrignal() {
        List<Person> siblings = familyTree.getRelationship(this.siblings);
        siblings.add(femalePerson);
        siblings.add(femalePerson);
        siblings.add(femalePerson);

        List<Person> siblingsAfter = familyTree.getRelationship(this.siblings);

        assertThat(siblingsAfter.size(), is(0));
    }

    @Test
    public void shouldRepresentFamilyTreeAsDesired(){
        familyTree.addRelationship(malePerson, siblings, femalePerson);
        familyTree.addRelationship(malePerson, siblings, femalePerson);
        System.out.println("familyTree = " + familyTree);
           }
}

[Junilu Lacar]

Well, it's kind of just what I suspected. None of these tests are very good in that they don't lay out a clear story of how People and FamilyTree are used from a non-"implementation aware" point of view.

Take this test, for example:
@Test public void shouldReturnFalseOnAddingIllegitimateRelationshipToFamilyTree() { when(siblings.isLegitimate(any(), any())).thenReturn(false); boolean result = familyTree.addRelationship(malePerson, siblings, femalePerson); assertFalse(result); }
What does isLegitimate(any(), any()) mean, semantically? I know it's a mock object that you're calling this on, but then that implies that this is a whitebox test that "knows" something about what goes on when the addRelationship() method is invoked. To introduce a mock in this test is saying "I expect the FamilyTree to invoke the siblings.isLegitimate() method as a result of having its addRelationship() method invoked. But what do the "any()" parameters mean? To me that says that "I expect siblings.isLegitimate() to always return false regardless of what two parameters are passed to it." That doesn't make sense to me.

The story around the name shouldReturnFalseOnAddingIllegitimateRelationshipToFamilyTree isn't very obvious either. Why are you expecting the result to be false? I know that's what the test method name says but why? When I see "illegitimate" in the context of families and siblings, I think of someone born out of wedlock. Those are the natural semantics that come to mind when you use "illegitimate" in close proximity with "sibling" and "relationship". So what does false mean as a result of adding an illegitimate relationship to a family tree?

I could make many of the same kind of comments about the other tests. Bottom line is, they don't present a very good definition of a coherent and cohesive API that makes sense.

[Junilu Lacar]

Here's something that I think would make more sense:

@Test
public void test() {
   Person joe = new Person("Joe", "Jackson");
   Person tito = new Person("Tito", "Jackson");
   Person jermaine = new Person("Jermaine", "Jackson");
   Person michael = new Person("Michael", "Jackson");
   Person bo = new Person("Bo", "Jackson");
   Person andrew = new Person("Andrew", "Jackson");

   FamilyTree jacksonsOfGary = new FamilyTree();
   jacksonsOfGary.setPatriarch(joe);
   jacksonsOfGary.addChild(joe, tito);
   jacksonsOfGary.addChild(joe, jermaine);
   jacksonsOfGary.addChild(joe, michael);
   
   assertTrue(jacksonsOfGary.isRelated(joe, jermaine);
   assertTrue(jacksonsOfGary.isRelated(tito, michael);
   assertTrue(jacksonsOfGary.isRelated(michael, germaine);

   assertFalse(jacksonsOfGary.isRelated(bo, michael);
   assertFalse(jacksonsOfGary.isRelated(michael, andrew);
   assertFalse(jacksonsOfGary.isRelated(bo, andrew);
}

Now, granted that this test is a bit long and the name sure could use some refactoring, doesn't it make more sense what the Person and FamilyTree objects are supposed to do even when you don't have a single clue as to how the methods that you see, setPatriarch(), addChild(), and isRelated() are doing what they apparently are able to do based on the story that the test is telling? Isn't the test code telling a coherent, understandable story?

NOTE: If you're unfamiliar with who most of these people are, you can probably figure out most of it if I tell you that Michael (the late, great, "King of Pop") Jackson's family is from Gary, Indiana. Bo Jackson was a well-known sports figure who was most popular in the 80s and early 90s. As far as I know, he's not related to the Jacksons of Gary, Indiana.

And it's easy enough to refactor that test name and identify a way to shorten up this test or maybe split it up into two separate tests.

@Test
public void isRelated_should_be_true_when_both_people_are_in_the_tree() {
   ...
}

@Test
public void isRelated_should_be_false_when_either_person_not_in_the_tree() {
   ...
}

@Test
public void isRelated_should_be_commutative() {
  ...
}

@Test
public void isRelated_should_be_transitive() {
  ...
}

These tests will define class behavior from an API perspective, not from an implementation perspective. I don't know if it's obvious what "isRelated should be commutative" and "isRelated should be transitive" means but if you see some code, I bet you'll understand it. Or maybe you do get what these mean and you can easily provide the test code that will exercise that property of relationships.

Here's what you need to keep in mind when you're doing TDD: Think API and semantics before you think about implementation.

A corollary to that is this: The danger of using mocks too early on when you're doing TDD is that mocks can lead to think about implementation before you have a coherent and well thought-out API.

[Junilu Lacar]

From the sample tests that I gave, how difficult would it be now to come up with more aspects of the API that make sense?

I can think of a number of possible ones right off the bat:

FamilyTree:

void setMatriach(Person p);
List<Person> getGenerationMembers(int level);
List<Person> getParentsOf(Person p);
List<Person> getSiblingsOf(Person p);
List<Person> getChildrenOf(Person p);
List <Person> getSpousesOf(Person p);
void setSpouseOf(Person p, Person s);
void setSpouseOf(Person p, Person s, TimePeriod t);
or

List <Person> getRelativesOf(Person p, Relationship r);

Just from these candidate methods in the API alone, you could probably come up with dozens of new unit tests to drive the development of the production code.

Is there an API for Person that you can think of to write tests for? I don't know. See what you can come up with. If not, then that's fine, too. Maybe that should lead you to ask whether it's even useful to have a Person be represented as a class. But then again, I haven't tried to implement any production code that would make some of these tests pass. Maybe in the course of implementing the design laid out by the tests, I'll find that I need Person.compareTo() or Person.equals(), or something like that. You'll only know if you try to write the code and see it right in front of you, not by conducting endless thought experiments.

[Junilu Lacar]

You might even play around with an idea of the FamilyTree being a Factory for Person objects that belong to it. That way, you could have this kind of code:

FamilyTree jacksonsOfGary = new FamilyTree("Jackson");

Person joe = jacksonsOfGary.newMember("Joe", "Jackson");
jacksonsOfGary.setPatriarch(joe);

Person tito = jacksonsOfGary.newMember("Tito", "Jackson");
Person jermaine = jacksonsOfGary.newMember("Jermaine", "Jackson");
Person michael = jacksonsOfGary.newMember("Michael", "Jackson");

joe.addChildren(tito, jermaine, michael);

Maybe that makes more sense. I'd play around with more test code to see how well that API works and if it makes more sense than always going through the FamilyTree. The way to tell from a technical perspective is to look out for code smells related to responsibility assignment in the FamilyTree code. Besides the Factory Method pattern, this API makes me think about the Visitor pattern because "callback method" comes to mind when I try to imagine the implementation details of some of these methods.

[Junilu Lacar]

Then, if you're thinking of using Java 8 lambdas and functional interfaces, the possibilities for a rich API expand even further.

Think how coherent an API that allowed you to do the following would be:

assertTrue(joe.is(fatherOf(michael));
assertTrue(michael.is(brotherOf(tito));
assertTrue(tito.is(brotherOf(michael));

assertFalse(joe.is(fatherOf(bo));
assertFalse(bo.is(sonOf(joe));

where fatherOf(), brotherOf(), and sonOf() are perhaps static methods in a Relationships class, designed kind of in the same way that the Java 8 Collectors class is designed. I bet it would be fun to explore a way to implement the code behind this API.

[Sangel Kapoor]

Junilu Lacar wrote:Well, it's kind of just what I suspected. None of these tests are very good in that they don't lay out a clear story of how People and FamilyTree are used from a non-"implementation aware" point of view.

Take this test, for example:
@Test public void shouldReturnFalseOnAddingIllegitimateRelationshipToFamilyTree() { when(siblings.isLegitimate(any(), any())).thenReturn(false); boolean result = familyTree.addRelationship(malePerson, siblings, femalePerson); assertFalse(result); }
What does isLegitimate(any(), any()) mean, semantically? I know it's a mock object that you're calling this on, but then that implies that this is a whitebox test that "knows" something about what goes on when the addRelationship() method is invoked. To introduce a mock in this test is saying "I expect the FamilyTree to invoke the siblings.isLegitimate() method as a result of having its addRelationship() method invoked. But what do the "any()" parameters mean? To me that says that "I expect siblings.isLegitimate() to always return false regardless of what two parameters are passed to it." That doesn't make sense to me.

The story around the name shouldReturnFalseOnAddingIllegitimateRelationshipToFamilyTree isn't very obvious either. Why are you expecting the result to be false? I know that's what the test method name says but why? When I see "illegitimate" in the context of families and siblings, I think of someone born out of wedlock. Those are the natural semantics that come to mind when you use "illegitimate" in close proximity with "sibling" and "relationship". So what does false mean as a result of adding an illegitimate relationship to a family tree?

I could make many of the same kind of comments about the other tests. Bottom line is, they don't present a very good definition of a coherent and cohesive API that makes sense.

I think you are Correct only if I had not written tests for Relationship classes.
If supposes I change isLegitimate method to take any parameter (as you said ), then my tests should fail. In my case Unit tests for Relationship classes will fail which is more relevant than failing family tree Unit tests.

Read it as, I don't care what isLegitimate takes , what I care is only return value and given that return value from stub, I am desiring some behaviour from Family Tree which is the sole purpose of Unit tests.

[Sangel Kapoor]

Junilu Lacar wrote:
What does isLegitimate(any(), any()) mean, semantically? I know it's a mock object that you're calling this on, but then that implies that this is a whitebox test that "knows" something about what goes on when the addRelationship() method is invoked. To introduce a mock in this test is saying "I expect the FamilyTree to invoke the siblings.isLegitimate() method as a result of having its addRelationship() method invoked. But what do the "any()" parameters mean? To me that says that "I expect siblings.isLegitimate() to always return false regardless of what two parameters are passed to it." That doesn't make sense to me.

The story around the name shouldReturnFalseOnAddingIllegitimateRelationshipToFamilyTree isn't very obvious either. Why are you expecting the result to be false? I know that's what the test method name says but why? When I see "illegitimate" in the context of families and siblings, I think of someone born out of wedlock. Those are the natural semantics that come to mind when you use "illegitimate" in close proximity with "sibling" and "relationship". So what does false mean as a result of adding an illegitimate relationship to a family tree?

I could make many of the same kind of comments about the other tests. Bottom line is, they don't present a very good definition of a coherent and cohesive API that makes sense.

if you try to add Male as spouse of another male , it is illegitimate. I am not sure in what directions you are thinking, how does these tests are not conveying you the meaning. These are Unit Tests and not integration Tests. It just deals with Family Tree responsibility of saving and retrieving relationships.
In my humble opinion it is telling me the story .

[Sangel Kapoor]

Junilu Lacar wrote:Here's something that I think would make more sense:

@Test
public void test() {
   Person joe = new Person("Joe", "Jackson");
   Person tito = new Person("Tito", "Jackson");
   Person jermaine = new Person("Jermaine", "Jackson");
   Person michael = new Person("Michael", "Jackson");
   Person bo = new Person("Bo", "Jackson");
   Person andrew = new Person("Andrew", "Jackson");

   FamilyTree jacksonsOfGary = new FamilyTree();
   jacksonsOfGary.setPatriarch(joe);
   jacksonsOfGary.addChild(joe, tito);
   jacksonsOfGary.addChild(joe, jermaine);
   jacksonsOfGary.addChild(joe, michael);
   
   assertTrue(jacksonsOfGary.isRelated(joe, jermaine);
   assertTrue(jacksonsOfGary.isRelated(tito, michael);
   assertTrue(jacksonsOfGary.isRelated(michael, germaine);

   assertFalse(jacksonsOfGary.isRelated(bo, michael);
   assertFalse(jacksonsOfGary.isRelated(michael, andrew);
   assertFalse(jacksonsOfGary.isRelated(bo, andrew);
}

Now, granted that this test is a bit long and the name sure could use some refactoring, doesn't it make more sense what the Person and FamilyTree objects are supposed to do even when you don't have a single clue as to how the methods that you see, setPatriarch(), addChild(), and isRelated() are doing what they apparently are able to do based on the story that the test is telling? Isn't the test code telling a coherent, understandable story?

NOTE: If you're unfamiliar with who most of these people are, you can probably figure out most of it if I tell you that Michael (the late, great, "King of Pop") Jackson's family is from Gary, Indiana. Bo Jackson was a well-known sports figure who was most popular in the 80s and early 90s. As far as I know, he's not related to the Jacksons of Gary, Indiana.

And it's easy enough to refactor that test name and identify a way to shorten up this test or maybe split it up into two separate tests.

@Test
public void isRelated_should_be_true_when_both_people_are_in_the_tree() {
   ...
}

@Test
public void isRelated_should_be_false_when_either_person_not_in_the_tree() {
   ...
}

@Test
public void isRelated_should_be_commutative() {
  ...
}

@Test
public void isRelated_should_be_transitive() {
  ...
}

These tests will define class behavior from an API perspective, not from an implementation perspective. I don't know if it's obvious what "isRelated should be commutative" and "isRelated should be transitive" means but if you see some code, I bet you'll understand it. Or maybe you do get what these mean and you can easily provide the test code that will exercise that property of relationships.

Here's what you need to keep in mind when you're doing TDD: Think API and semantics before you think about implementation.

A corollary to that is this: The danger of using mocks too early on when you're doing TDD is that mocks can lead to think about implementation before you have a coherent and well thought-out API.

I feel various issues in above Test

1. There is no need to make Person Specifically, because family tree has nothing to take with person names. You should mock.
2. You are always adding relationship to family tree , no matter what the sex of person is and whether caller is calling in a right way or not. So you will addHusband even when person is female.
3. This API seems different in a way I am solving this problem. Probably design in your mind is different from mine.

[Sangel Kapoor]

This is how API looks from Client

Person joe = person("Joe","A",Sex.MALE);
Person michael = person("Michael","B",Sex.FEMALE);
Person isha = person("Isha","C",Sex.FEMALE);


registrar.addRelationship(joe, Relationships.FATHER,nirmal);
registrar.addRelationship(joe, Relationships.MOTHER,isha);
registrar.addRelationship(joe, Relationships.SIBLING,varun);
registrar.addRelationship(joe, Relationships.SPOUSE,michael);


System.out.println("Joe Family Tree \n" + joe.getFamilyTree());
        
 public static Person person(String firstName, String lastName, Sex sex){
        Name name = name(firstName,lastName);
        PersonalInformation personalInformation = PersonalInformation.personalInformation(name,sex);
        return Person.person(UUID.randomUUID(),personalInformation, hashBasedFamilyTree(new HashMap<>()), registrar);
    }

    public static Name name(String firstName, String lastName){
        return Name.name(FirstName.firstName(firstName), LastName.lastName(lastName));
    }

Also , when there is a birth of a child, you go to "Some Government Office" and registers your relationship legally.
So, does not api gives you that feel ??

[Junilu Lacar]

Sangel Kapoor wrote:if you try to add Male as spouse of another male , it is illegitimate. I am not sure in what directions you are thinking, how does these tests are not conveying you the meaning. These are Unit Tests and not integration Tests. It just deals with Family Tree responsibility of saving and retrieving relationships.
In my humble opinion it is telling me the story .

Well, if you want your FamilyTree class to take that side of the global social issue of same-sex marriage, I suppose that's your prerogative. However, the test code does not explicitly say anything about spouse; it only deals with male, siblings, female. As such the intent that you state is not clearly communicated in the code that you wrote.

I stand by my assertion that your test code does not tell a clear and coherent story and that these are making the API design unclear and incoherent as well. The distinction between unit vs integration test is irrelevant. What's relevant is white box vs black box. White box tests have knowledge of implementation details, which is what using mock objects implies. These are more brittle and more implementation-specific than black box tests. You have white box tests and I think it's too early for those at this point.

You said previously that you were doing this design with "your team". What do other team members say? Do they find the API you have makes sense? Your reaction of defending your design rather than considering the possibility that it is in fact flawed is natural but it also makes me doubt there's really a team effort involved in coming up with the design. If there is indeed a team involved here then all I can say is "Good luck."

Regarding your comment on the birth registrar, I think you are falling into a trap of trying to model the real world too closely. By your logic, if I were designing a Car class, I would also have to introduce a GasStation class so I can fill up my Car with fuel or a BMV class (Bureau of Motor Vehicles) so I can register my vehicle. A birth registrar has nothing to do with drawing up a family tree, even in the real world. I could sit down with a group of my relatives and we could try to draw up our family tree as best as we could without the idea of a birth registrar coming up a single time in that entire conversation.

IMO, you're taking "modeling" a bit too far. OO design is not about reflecting the real world in your program, it's about assigning responsibilities correctly. You're also thinking about implementation too much and it's clouding your objectivity about the clarity of your API. I'm not saying that the alternatives I gave are perfect but at least they are strictly from an external point of view that doesn't incorporate any assumptions about implementation details. That gives me freedom to play around with different alternatives for the API rather than paint me into a corner which is what I think has happened to you with the kind of tests you're writing.

[Junilu Lacar]

And please don't take any of my comments about your design personally. I try to be as objective as I can be but given that I have my own ideas of how to go about creating a solution, there's also an unavoidable inherent bias so take my comments with a grain of salt.

[Junilu Lacar]

Sangel Kapoor wrote:
1. There is no need to make Person Specifically, because family tree has nothing to take with person names. You should mock.

Why should I think about mocking at all? That will just pull me prematurely into thinking about implementation details. Mocks are meant to be used when the class under test has collaborators involved. You're only supposed to be thinking about API at this point. If your API design makes assumptions about collaborations, then you're leaking implementation details and that's not good.

2. You are always adding relationship to family tree , no matter what the sex of person is and whether caller is calling in a right way or not. So you will addHusband even when person is female.

You're mixing design concerns here. Thinking about error conditions is fine but keep it separate. The examples I gave are "happy path" scenarios meant to explore various options for a sensible API. I was focusing on what could be done first, not want should not be allowed. Thinking about error conditions would come later, after settling on an API that looked good enough to move forward with.

3. This API seems different in a way I am solving this problem. Probably design in your mind is different from mine.

I think that's quite obvious and I couldn't agree more.

[Junilu Lacar]

Here's where I think you're falling short: thinking about usage scenarios.

You're thinking too deeply about classes, attributes, error conditions, etc. Again, I think it's too early for that level of thinking. You have to think at a higher, more abstract level.

Here's what I had in mind when I was writing those examples:

1. If I had a single family, how would I draw up their family tree? Let's take the Jackson family for example. Joe Jackson is the father and Tito, Jermaine, and Michael are his sons. Ok, let's see what kind of API would make sense to establish a family tree for these guys.
2. I'd like to see if someone is related to someone else. Ok, then if I had the Jackson family tree set up, then it should be able to tell me if it has information about Joe, Tito, Jermaine, and Michael being related to each other. Furthermore, if I had Bo Jackson, the family tree should be able to tell me that Bo is not related to the other Jacksons because Bo is not in that family tree.

This is the narrative that's reflected in those example tests I gave.

Draw up some high-level usage scenarios for using the API that you had in mind first. Then make some objective, critical evaluations, keeping in mind that most people don't come up with a good design with their first few tries. You saw that above when I started exploring alternative API designs right after coming up with that first one.

Even if you want to consider error conditions first, avoid mocks and don't think too much about implementation. Try to keep things small for now. By limiting the API to addChild(Person) instead of the wide open addRelationship(Person, Relationship, Person), I can focus on a simpler mental model rather than get confused with a more complex model that comes with the more generic API. With addChild(), I only have to think about and test the Parent-Child relationship. Once I have that established, I can make the small logical extension of considering sibling relationships based on a common Parent. If I then wanted to think about refining the Sibling relationship to Brother/Sister, then I can throw in Gender/Sex as part of the picture. The point is that I can grow the design incrementally and slowly add complexity from a solid base.

With addRelationship(), the complexity virtually explodes in my face right from the start. Why try to fly when you haven't even learned how to crawl yet?

[Junilu Lacar]

Sangel Kapoor wrote:This is how API looks from Client

 ...
 public static Person person(String firstName, String lastName, Sex sex){
        Name name = name(firstName,lastName);
        PersonalInformation personalInformation = PersonalInformation.personalInformation(name,sex);
        return Person.person(UUID.randomUUID(),personalInformation, hashBasedFamilyTree(new HashMap<>()), registrar);
    }

This exemplifies what I said about thinking too deeply about implementation details. UUID, HashMap, hashBasedFamilyTree, all these leak details about the implementation that you have in mind. These are the kind of things I don't worry about just yet during the early stages of API design.

[Junilu Lacar]

And if you're wondering why I use the word "leak", it's related to the idea of leaky abstractions.

[Junilu Lacar]

Sangel Kapoor wrote:
How can you represent this as a Tree is confusing me .
Binary or Not-binary does not matter at all.
In a Tree you have one Parent, and we have notion of Father and Mother both in FamilyTree we are taking about.
You might use forest for this, but still you cannot represent 2 parents.

You have boxed your mind into a fixed solution with your assumption that a node in the tree can only be a single Person.

This is what I was alluding to earlier when I said that marriage is when "Two become One."

If you want to take advantage of the convenience that a Tree structure can afford, then you can take a couple that produces one or more children as the Parent node. That is, the Parent node is comprised of two People who are the biological parents of one or more People. The "couple" relationship doesn't even have to be "legitimized" by the institution of marriage. Does being unmarried make two people any less the biological parents of their offspring? No, right?

Next, you'll probably wonder about belonging to different family trees. Well, that's the whole point of what I said about a FamilyTree being multi-dimensional. This is where separating and limiting concerns and viewpoints to simplify relationships that you're dealing with now comes in. Why force ourselves to deal with the complex? Why not find a way to reduce the complexity into something simpler and more manageable?

If you're thinking that it would be "wasteful" or "inefficient" to have the same Person added to different FamilyTrees, then you've again trapped yourself in a box. This is what I alluded to when I mentioned premature optimization limiting the options that you're able to consider or even realize are there.

I wrote:premature optimization and thinking about normalization can box your thinking into something that's not workable

Of course, the reality is that I can draw up separate family trees for my Father's side of the family and my Mother's side of the family and naturally, I would belong to both family trees. When you consider that I'm married and have children, my children belong to at least four separate family trees: the two that I belong to and the two that my wife belongs to. In fact, if you go up high enough, the number of trees that any person belongs to grows exponentially and you'll quickly realize that everyone on this planet could in fact be related to each other, however remotely, in one way or another. In fact, I just recently joined a group on FaceBook of people who belong to the family tree of my maternal grandfather. There are easily hundreds of new "relatives" that I have discovered there alone. I even saw one clan member who was obviously an American (caucasian). She had the same last name as my maternal grandfather.

So, if I see Nodes in the FamilyTree as being either a Couple made up of two people or a Single Person, then I absolutely could use a non-binary tree as my data structure, at least for one dimension. Connecting different trees with each other would probably involve a different data structure but again, I have simplified and abstracted so that I can deal with complexity better.

Sangel Kapoor wrote:Also FamilyTree might not be a Tree DataStructure , in the same way as you said word "Controller" does not mean same Controller in Design Patterns (MVC).

Actually, what I said was that if you say Controller in the context of design, then I naturally think about the Controller design pattern, not whatever it was that you were thinking about, like a Registrar concept maybe. I already commented about what I think about the Registrar.

=======
SIDEBAR: The thing about multiple family trees got me thinking about my younger sister's sons. My brother-in-law is caucasian and Jewish. Trying to imagine the combination of family trees that my nephews belong to and the diversity of their genealogy is pretty mind-boggling.
=======

[Winston Gutkowski]

Sangel Kapoor wrote:How can you represent this as a Tree is confusing me .
Binary or Not-binary does not matter at all.
In a Tree you have one Parent, and we have notion of Father and Mother both in FamilyTree we are taking about.
You might use forest for this, but still you cannot represent 2 parents.
Also FamilyTree might not be a Tree DataStructure...

OK, let's take those things one at a time:
1. I think the term Family Tree is a misnomer.
2. I'm not even sure it's a single structure, particularly when things like siblings get involved.
3. The "personal" part of it that IS a tree is that it still (AFAIK) takes two, and only two, people to produce a child; so you can think of each Person as being the root of an inverted binary tree.

You could also take Junlu's idea and use the Spouse relationship (which involves two people) as the root of an N-tree. The only problem then is that the product of that relationship is one or more people, so the tree would have to support at least two different types of Node.

It's certainly do-able; just not as simply as maybe you were thinking.

And then you get families like mine, where I have a half-brother and half-sister who aren't related.

Winston

[Junilu Lacar]

Winston Gutkowski wrote:
And then you get families like mine, where I have a half-brother and half-sister who aren't related.

Exactly

This is where you can build out complexity. I mentioned two major categories of relationships: biological and social/legal. The half-sibling relationship occurs because half of the couple that produced one Person is also half of a different couple that produced the half-sibling. Winston is biologically related to his half-brother who is the offspring of one couple and at the same time is biologically related to his half-sister who is the offspring of another couple. That's how the half-brother and half-sister are not biologically related at all because they come from separate halves of the couple that produced Winston.

Does that sound about right, Winston?

All this supports the idea that a Tree structure is useful because it inherently models the Parent-Child relationship. All other kinds of relationships can be calculated based on this most fundamental of relationships. Sangel and Paul were right: taken as a whole, the resulting data structure that represents multiple generations in a family tree is probably not going to be a Tree but rather a Graph. However, you can reduce the problem so that you're dealing mostly with Trees and then dealing with complexity by combining/jumping between Trees from nodes that represent Couples.

[Winston Gutkowski]

Junilu Lacar wrote:Does that sound about right, Winston?

Spot on.

Winston

[Campbell Ritchie]

If you really are going to model families, you end up with two trees; I have a tree with descendants in, and I also have a tree of all my ancestors. Obviously the tree of descendants is smaller.

[Sangel Kapoor]

Junilu Lacar wrote:Here's where I think you're falling short: thinking about usage scenarios.

You're thinking too deeply about classes, attributes, error conditions, etc. Again, I think it's too early for that level of thinking. You have to think at a higher, more abstract level.

Here's what I had in mind when I was writing those examples:

1. If I had a single family, how would I draw up their family tree? Let's take the Jackson family for example. Joe Jackson is the father and Tito, Jermaine, and Michael are his sons. Ok, let's see what kind of API would make sense to establish a family tree for these guys.
2. I'd like to see if someone is related to someone else. Ok, then if I had the Jackson family tree set up, then it should be able to tell me if it has information about Joe, Tito, Jermaine, and Michael being related to each other. Furthermore, if I had Bo Jackson, the family tree should be able to tell me that Bo is not related to the other Jacksons because Bo is not in that family tree.

This is the narrative that's reflected in those example tests I gave.

Draw up some high-level usage scenarios for using the API that you had in mind first. Then make some objective, critical evaluations, keeping in mind that most people don't come up with a good design with their first few tries. You saw that above when I started exploring alternative API designs right after coming up with that first one.

Even if you want to consider error conditions first, avoid mocks and don't think too much about implementation. Try to keep things small for now. By limiting the API to addChild(Person) instead of the wide open addRelationship(Person, Relationship, Person), I can focus on a simpler mental model rather than get confused with a more complex model that comes with the more generic API. With addChild(), I only have to think about and test the Parent-Child relationship. Once I have that established, I can make the small logical extension of considering sibling relationships based on a common Parent. If I then wanted to think about refining the Sibling relationship to Brother/Sister, then I can throw in Gender/Sex as part of the picture. The point is that I can grow the design incrementally and slowly add complexity from a solid base.

With addRelationship(), the complexity virtually explodes in my face right from the start. Why try to fly when you haven't even learned how to crawl yet?

I sincerely appreciate your efforts for explanation. I take your points and will keep in mind.

But I get confused when you say its too early to decide because what you are seeing is a result of 2 weeks re-factoring and not the initial design. I actually started from add a Particular relationship but after coding and eventually we realized the right choice would be to make addRelationship().
Nevertheless, I sincerely understood your points and will definitely keep them in mind.

Also just to mention, I started with design choices , I am not a Big Fan of doing strict-TDD. It does not seems natural to me while I code for complex projects where many classes are involved. Its feel only good to me when I see addSum demo example for TDD demonstration. Well that's separate topic though.

Also "OO is all about giving right responsibilities to classes and not mapping real world" .... not sure who has defined this RULE !.. and even if it is a RULE defined by someone, with due regards I disagree...not saying not to make cohesive classes but design should be as natural as possible.
The way you see, think and perceive should be reflected in code as much as possible. That's my opinion and that is how I code daily with same thoughts .

[Sangel Kapoor]

Campbell Ritchie wrote:If you really are going to model families, you end up with two trees; I have a tree with descendants in, and I also have a tree of all my ancestors. Obviously the tree of descendants is smaller.

This is Interesting

[Junilu Lacar]

TDD is difficult and there are many mental traps to avoid. Design is difficult and OO design is difficult and there are more mental traps to avoid. Mixing implementation thinking with abstract API thinking is just one trap that you seem to still be falling into. Mock objects are lures that invite you into this trap.

Relating concepts in your program to real world objects can be useful but it can also be a trap to try to model too closely to the real world. I already gave you an example with the Car and the GasStation / BMV. You have to know where to draw the line between real world and abstractions. It's more important to think about objects in terms of behaviors and responsibilities coupled with the information that they need to perform those behaviors and fulfill their responsibilities.

A List is something we can relate because most of us have experience with something similar in the real world. But where is the real world equivalent of asking a list for an Iterator? The Iterator is an abstract concept that makes it easier for us to visualize and compartmentalize a set of responsibilities and behaviors. It allows us to simplify and forget about implementation details. The abstraction is useful and makes sense in software even though there is no real world equivalent.

One of the keys to success in TDD is keeping things simple and working in small increments. Refactoring is also very important. From what I see, you still need some improvement in both these areas. Knowing and being very conscious about various design principles is also very important. Like I said, it's not easy.

Good luck.

[Sangel Kapoor]

Thanks Junilu , Appreciate your time and Efforts !!

Can you refer me some good design books wherein problem is given and then a Solution is also given rather than Theory.
I would like to solve more design problems so that I can feel your points more closely.

I also tried to think in abstract way and hence wrote Interface with abstract APIs first but still seems like you are not liking it, might be more more more design problems to solve !!! :-)

[Junilu Lacar]

No problem. I like these kinds of discussions. They make me think about the way I think and learn.

The books that I usually cite are:

1. 4 Rules of Simple Design by Corey Haines - best book about design and TDD that I've read so far
2. Agile Software Development - Principles, Patterns, and Practices by Robert "Uncle Bob" Martin
3. Refactoring - Improving the Design of Existing Code by Martin Fowler - because we always write bad code
4. Clean Code by Uncle Bob - because we always write code that could be cleaner
5. Working Effectively with Legacy Code by Michael Feathers - because we always write legacy code, even that code you wrote just a few minutes ago.
6. Refactoring to Patterns by Joshua Kerievsky - because you should think about refactoring before you think about patterns.

These are my mainstays and in fact, I have a couple of them on my desk right now and the rest are just a few clicks of the mouse away on my hard drive.

Other authors whose books I find useful:

- Craig Larman - I like his GRASP (General Responsibility Assignment Software Principles) - he has a book about OO design and analysis with UML. I don't use UML much but I do find his design and analysis techniques useful.

- Peter Coad, et. al - I have their book, Java Modeling in Color with UML, on my desk right now. This one I use not for the UML, which I pretty much ignore, but for other ideas like Archetypes and Feature-Driven Development and a number of useful modeling tips that I like to remind myself about once in a while. For example, "Repeatedly asking for state? If you find an object repeatedly asking objects it links to what state they are in, let the object hold state-specific collections. That simplifies the design and reduces message traffic" and "When to use inheritance? Use inheritance to express specialized moment-intervals, descriptions, or party/place/things" and "When not to use inheritance? Not for 'is a role played by a'. Not for changes of what you know about something over time (use moment-intervals to show that progression). Not to factor out an algorithm (use a plug-in-point instead)."

That last tip about when not to use inheritance might be a good one for you to consider.

There are many more on my bookshelf that I'll refer to once in a while but these are the ones that I use most often.

[Junilu Lacar]

Sangel Kapoor wrote:Interface with abstract APIs first but still seems like you are not liking it

Be more concerned about the people who will be using your API. Is the API intuitive? Is it abstract enough? Whether or not I like it is not important although it will probably give an indication of how others will feel, too.

[Junilu Lacar]

@Test public void shouldAddNewLegitimateRelationshipToFamilyTree() { familyTree.addRelationship(malePerson, siblings, femalePerson); List<Person> spousesAfterFirstAddition = familyTree.getRelationship(siblings); assertThat(spousesAfterFirstAddition.size(), is(1)); assertTrue(spousesAfterFirstAddition.get(0).equals(femalePerson)); ... }
Just one more example, to help you understand why I don't feel this is a very good test or API.

On line 55 you invoke the addRelationship method, passing in a male, a relationship called 'siblings', and a female.
Then on line 56 you ask the family tree for a relationship that somehow involves the relationship, siblings and you call this list spousesAfterFirstAddition

If I were to read this out loud in an attempt to understand the "story" you're trying to tell with this code, it would be like this:

First, you establish a sibling relationship in the family tree between this male and female. So the male and female are brother and sister (because of the siblings relationship)
Then you ask the family tree to give back the list of people that are involved in the siblings relationship.
Why are you calling it spousesAfterFirstAddition? Are you expecting the brother and sister to be considered as spouses now? WTH?
Next, on line 58, we're asserting that the list should only have one Person in it. What? Why? When you created the relationship, there were TWO people involved. How can we expect only ONE person to be returned now when we query for the same relationship?
Next, on line 59, we're asserting that the person in the list should be the female. Why the female? Why not the male? Why not both?

So, it takes a little bit of analysis to realize that you're thinking about "siblings" in terms of nodes in a tree and their relative positions. That is, your design of a husband and wife "couple" is that they will be two sibling nodes in the graph/tree. But in a Graph, the concept of "sibling" makes no sense. There's just "neighbor" on a Graph. A Tree has a logical hierarchy with parent-child relationships, with sibling nodes being child nodes that have the same parent node.

But you're mixing the implementation semantics of sibling with the semantics that are naturally associated with the abstract concept of human relationships in a family tree. When you say "sibling" in the abstract sense, that means brother or sister. In your code, "sibling" refers to the implementation semantics of tree nodes rather than the abstract semantics.

This is why your test code is confusing and it's muddying up your API as well as your design thinking around your API.

[Junilu Lacar]

And before you try to explain more clearly what you intended, that's exactly the point I'm trying to make about your API and test code not being intuitive. If it were intuitive, you wouldn't have to explain it all and anyone reading the code who didn't have a psychic connection to your brain would know exactly what you were thinking.

[Sangel Kapoor]

Junilu Lacar wrote:And before you try to explain more clearly what you intended, that's exactly the point I'm trying to make about your API and test code not being intuitive. If it were intuitive, you wouldn't have to explain it all and anyone reading the code who didn't have a psychic connection to your brain would know exactly what you were thinking.

[Sangel Kapoor]

Junilu Lacar wrote:@Test public void shouldAddNewLegitimateRelationshipToFamilyTree() { familyTree.addRelationship(malePerson, siblings, femalePerson); List<Person> spousesAfterFirstAddition = familyTree.getRelationship(siblings); assertThat(spousesAfterFirstAddition.size(), is(1)); assertTrue(spousesAfterFirstAddition.get(0).equals(femalePerson)); ... }
Just one more example, to help you understand why I don't feel this is a very good test or API.

On line 55 you invoke the addRelationship method, passing in a male, a relationship called 'siblings', and a female.
Then on line 56 you ask the family tree for a relationship that somehow involves the relationship, siblings and you call this list spousesAfterFirstAddition

If I were to read this out loud in an attempt to understand the "story" you're trying to tell with this code, it would be like this:

First, you establish a sibling relationship in the family tree between this male and female. So the male and female are brother and sister (because of the siblings relationship)
Then you ask the family tree to give back the list of people that are involved in the siblings relationship.
Why are you calling it spousesAfterFirstAddition? Are you expecting the brother and sister to be considered as spouses now? WTH?
Next, on line 58, we're asserting that the list should only have one Person in it. What? Why? When you created the relationship, there were TWO people involved. How can we expect only ONE person to be returned now when we query for the same relationship?
Next, on line 59, we're asserting that the person in the list should be the female. Why the female? Why not the male? Why not both?

So, it takes a little bit of analysis to realize that you're thinking about "siblings" in terms of nodes in a tree and their relative positions. That is, your design of a husband and wife "couple" is that they will be two sibling nodes in the graph/tree. But in a Graph, the concept of "sibling" makes no sense. There's just "neighbor" on a Graph. A Tree has a logical hierarchy with parent-child relationships, with sibling nodes being child nodes that have the same parent node.

But you're mixing the implementation semantics of sibling with the semantics that are naturally associated with the abstract concept of human relationships in a family tree. When you say "sibling" in the abstract sense, that means brother or sister. In your code, "sibling" refers to the implementation semantics of tree nodes rather than the abstract semantics.

This is why your test code is confusing and it's muddying up your API as well as your design thinking around your API.

spousesAfterFirstAddition : This is Typo occurred because Test cases were written first with Spouse Relationship and then I moved it to Sibling relationship and somehow forgot to change these variable names in jot. Read it as siblingsAfterFirstAddition.

I got your point entirely, probably my Tests are not so declarative and intuitive. familyTree.addRelationship(Person myself, Relationship relationship, Person second) is working on adding second person as a Relative to family tree of first person.
So when I want to retrieve spouse of first Person , it should be one.

In my API, every person hash his own family tree which holds all the people to whom I am related to . So for particular person if you ask his/her family tree to get SPOUSE : it will give his/her spouse, SIBLING : it will give you list of his siblings, ....... this was exactly the Original Spec was !!

[Sangel Kapoor]

See these 3 methods to understand what I am trying to achieve. These methods belong to Singleton FamilyRegistrar.
Clinets are expected to add Relationships via Registrar to their family trees.

public boolean addRelationship(Person first, Relationship relationship, Person second) throws IllegalRelationshipException {
        addRelationshipWithFirstPerson(first, relationship, second);
        addReverseRelationshipWithSecondPerson(first, relationship, second);
        return true;
    }

    private void addRelationshipWithFirstPerson(Person first, Relationship relationship, Person second) throws IllegalRelationshipException {
        FamilyTree firstPersonFamilyTree = first.getFamilyTree();
        boolean added  = firstPersonFamilyTree.addRelationship(first,relationship,second);

        if(!added) throw illegalRelationshipException(ILLETIMATE_RELATIONSHIP_MESSAGE);
    }

    private void addReverseRelationshipWithSecondPerson(Person first, Relationship relationship, Person second) throws IllegalRelationshipException {
        FamilyTree secondPersonFamilyTree = second.getFamilyTree();
        Relationship reverseRelationShip = relationship.getReverseRelationship(first,second);
        secondPersonFamilyTree.addRelationship(second,reverseRelationShip,first);
    }

If you are thinking why I am passing first Person (whose Family Tree is to be Updated) , because I need to add first person in Second person family tree too with reverseRelationship. This ReverseRelationship I will get from Relationship Instance depending on Sex of first person.

[Junilu Lacar]

Sorry, but none of those explanations make it any better for me. You can either choose to continue on the path you're on right now and see how well users of your API take to it or pivot and rethink the API. Honestly, the new code you shared still has a lot of problems. Just about the one thing good I have to say about it is that the methods are short. Short and cryptic is better than long and cryptic. It's still cryptic though.

Here's what I mean about cryptic:

addRelationshipWithFirstPerson(first, relationship, second);

At first glance, the question comes up, "Where are we adding this relationship?" This looks like procedural code rather than object-oriented code. So we trace it down to that method and see what's going on. Now we see that we're really adding it to the family tree of the first person. Ok, that's fine. Going back up one call level, we trace it back down to setting the reverse relationship. Now we're adding the relationship into the second person's family tree. Are there two trees involved now? Why not just one tree? If you have a husband and wife, I can understand two trees but what about brother and sister? Wouldn't you just have one tree? Seems overly complicated to me.

Edit: Had to do a double take there to make sure I wasn't seeing a recursive call. The FamilyTree class appears to have an addRelationship() method that has the same signature as the addRelationship() method you show there. I can only assume that this method is supposed to be in that Registrar class you keep referring to. I think you need to learn how to let go and throw away a design that's smelly. If you don't learn that skill, you're just walking around while holding the bag, except in this case you really are the one to blame. I would dump the Registrar class if I were you and move responsibilities back to the FamilyTree.

So what happens when we set up the relationship between two parents and their children? Does it still work the same way? How do we establish a relationship between an Uncle and Nephew? How do we figure out whether the uncle is on the maternal side or the paternal side? What about cousins? How do I know who my first cousins are? My second cousins? The API quickly becomes intractable when you start trying to answer these questions and you get an explosion of relationships.

I will again make the assertion that once you establish the foundational Parent-Child relationship, all other relationships can be calculated based on this. Therefore, there is really no need for this Relationship class in the way that you have designed it.

[Junilu Lacar]

I wrote:

addRelationshipWithFirstPerson(first, relationship, second);

... This looks like procedural code rather than object-oriented code.

If, on the other hand, it had been like this:

first.setRelationship(relationship, second);
second.setRelationship(relationship.reverse(), first);

that would make it look more OO than procedural. I still don't think a Relationship class like this is needed though.

There are more questions that would arise even with this code. Like why do you need separate calls. Wouldn't you just need to call setRelationship on either of the first or second object then that object would take care of setting up the reverse relationship with the other object?

That is, you could push down the responsibility of setting up the reverse relationship:

// In Person
public void setRelationship(Relationship r, Person p) {
   this.tree.add(r, p);
   if (p.isNotRelated(r.reverse(), this)) {
      p.setRelationship(r.reverse(), this);
   }
}

The check is to prevent infinite recursion, making sure you only set the reverse relationship if it hasn't already been established. This way, you could call setRelationship() just one time and have that call automatically take care of establishing the reverse relationship.

Refactoring once more for a smell (violates Tell, Don't Ask):

// In Person
public void setRelationship(Relationship r, Person p) {
   if (this.hasRelationship(r, p))
      return;
      
   this.tree.add(r, p);
   p.setRelationship(r.reverse(), this);
}

I have to repeat that the idea of a Relationship type used this way is smelly to me.

[Dave Tolls]

Got to agree re: Relationship.
It's too prescriptive.

Beyond knowing X and Y are parents of Z pretty much every other relationship can be calculated.
Want to know how many siblings Z has? Then find out who the children of Y's parents are.

If I add a child to X I most certainly do not want to go through all of X and Y's children and assigning a new relationship to this new kid.

Even the above is prescriptive (and I'm only using it as an example) as adoption needs to be taken into account somehow or you'll have people vanish who should be there.
Families are messy things, and the more rules you put in place to cast relationships in stone then the harder it will be for people to fit reality into your model.

I've seen simple things like remarriages being a pain in some freebie genealogy software (for example), resulting in a step-parent who has no kids simply not appearing in the tree, which seems an odd thing to do. They may not be part of the DNA line, but that's no reason to have them vanish from the picture like some Stalinist bit of photo-editing...

[Winston Gutkowski]

Dave Tolls wrote:Got to agree re: Relationship.
It's too prescriptive.
Beyond knowing X and Y are parents of Z pretty much every other relationship can be calculated.
Want to know how many siblings Z has? Then find out who the children of Y's parents are.

Hmmm. Disagree. That only gives you "half" the result.

If "sibling" means "full sibling" then it will be children common to both X and Y; if not, then it will be the union of the children of both X and Y (I'm assuming here that "sibling is an operation done for a specific Person).

But since I was the one who suggested a Relationship, let me defend my reasoning.

A Person class might be created for any number of reasons, but the only place you're likely to need to know (or store) information about a Person's children is for an application like this.

That says to me that we're dealing with a specialization of a Person (Parent?).

The problem then is that if you implement a Parent type containing a list of children, it only deals with the relationship in one direction - downwards - and trying to work out who is the parent (or indeed any antecedent) of child Z gets quite hairy.

Secondly, you end up storing a lot of redundant information that may not be easy to maintain. Now I have to admit that my solution comes from my RDB background, where many:many relationships have to be implemented with an "associative entity", which may not be necessary for a language where links are by reference.

However, if that's true, then you could simply create something like

public class Individual
   extends Person
{
   private final Individual father;
   private final Individual mother;
   private final Set<Individual> spouses; // or should that be 'spice'?
   private final Set<Individual> children;
   ...
}

However, I wouldn't want to be the person (no pun intended) that has to maintain a collection of them.
Where would you start?

Winston

[Junilu Lacar]

Parent is a role, as is son, brother, sister, cousin, etc.

One person can have multiple roles depending on the context, just as you are half-brother, son, and stepson to different people.