1
\$\begingroup\$

I have written a class that represents humans. The class will serve as the basis for creating a program for managing and analyzing family trees. However, before using this class as the basis for my project, I would like to ask if my class is useful for this purpose, or if I should implement certain things differently. I have already tested this class by creating a test class that analyzes my own family tree. So far it has not come to mistakes. For privacy reasons, I cannot publish the test class here.

It was particularly important to me to program the class in high-performant and in a clear manner so that other people could also use it for their own projects. With regard to these aspects I wish for a code review of my class.

package main;

import java.util.List;
import java.util.Set;
import java.util.ArrayList;
import java.util.HashSet;

public class Human {
    private String firstName;
    private String lastName;
    private boolean male;
    private Human father;
    private Human mother;
    private Set<Human> children;
    private Human marriagePartner;

    public Human() {
        children = new HashSet<>();
    }

    public Human(String firstName, String lastName, boolean male) {
        this();
        this.firstName = firstName;
        this.lastName = lastName;
        this.male = male;
    }

    @Override
    public String toString() {
        return firstName + " " + lastName;
    }

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }

    public boolean isMale() {
        return male;
    }

    public boolean isFemale() {
        return !male;
    }

    public void setMale(boolean male) {
        this.male = male;
    }

    public Human getFather() {
        return father;
    }

    public void setFather(Human father) {
        this.father = father;
        father.addChildrenWithoutSettingParents(this);
    }

    private void setFatherWithoutAddingChild(Human father) {
        this.father = father;
    }

    public Human getMother() {
        return mother;
    }

    public void setMother(Human mother) {
        this.mother = mother;
        mother.addChildrenWithoutSettingParents(this);
    }

    private void setMotherWithoutAddingChild(Human mother) {
        this.mother = mother;
    }

    public List<Human> getChildren() {
        List<Human> childrenList = new ArrayList<>();
        childrenList.addAll(children);
        return childrenList;
    }

    public void setChildren(List<Human> children) {
        this.children = new HashSet<>(children);
    }

    public void addChildren(Human... children) {
        for (Human child : children) {
            if (male) {
                child.setFatherWithoutAddingChild(this);
            } else {
                child.setMotherWithoutAddingChild(this);
            }
            this.children.add(child);
        }
    }

    private void addChildrenWithoutSettingParents(Human... children) {
        for (Human child : children) {
            this.children.add(child);
        }
    }

    public Human getMarriagePartner() {
        return marriagePartner;
    }

    public boolean setMarriagePartner(Human marriagePartner) {
        if (marriagePartner.isMale() != marriagePartner.isMale()) {
            this.marriagePartner = marriagePartner;
            if (isFemale()) {
                setLastName(marriagePartner.getLastName());
            }
            return true;
        }
        return false;
    }

    public Human[] getGrandFathers() {
        Human[] grandFathers = new Human[2];
        if (mother != null) {
            grandFathers[0] = mother.getFather();
        }
        if(father != null) {
            grandFathers[1] = father.getFather();
        }
        return grandFathers;
    }

    public Human[] getGrandMothers() {
        Human[] grandMothers = new Human[2];
        if (mother != null) {
            grandMothers[0] = mother.getMother();
        }
        if(father != null) {
            grandMothers[1] = father.getMother();
        }
        return grandMothers;
    }

    public List<Human> getSiblings() {
        Set<Human> siblings = new HashSet();
        if (father != null) {
            for (Human child : father.getChildren()) {
                if (child != this) {
                    siblings.add(child);
                }
            }
        }
        if (mother != null) {
            for (Human child : mother.getChildren()) {
                if (child != this) {
                    siblings.add(child);
                }
            }
        }

        List<Human> siblingsList = new ArrayList<>();
        siblingsList.addAll(siblings);
        return siblingsList; 
    }

    public List<Human> getSiblingsOfParents() {
        Set<Human> siblings = new HashSet<Human>();
        // get children of grandFather except own father
        for (Human grandFather : getGrandFathers()) {
            if (grandFather != null) {
                for (Human children : grandFather.getChildren()) {
                    if (children != father && children != mother) {
                        siblings.add(children);
                    }
                }
            }
        }

        // get children of grandFather except own father
        for (Human grandMother : getGrandMothers()) {
            if (grandMother != null) {
                for (Human children : grandMother.getChildren()) {
                    if (children != father && children != mother) {
                        siblings.add(children);
                    }
                }
            }
        }

        List <Human> siblingsList = new ArrayList<>();
        siblingsList.addAll(siblings);
        return siblingsList;
    }

    public List<Human> getUncles() {
        List<Human> siblings = new ArrayList<>();
        siblings.addAll(getSiblingsOfParents());

        List<Human> uncles = new ArrayList<Human>();
        for (Human sibling : siblings) {
            if (sibling.isMale()) {
                uncles.add(sibling);
            }
        }

        return uncles; 
    }

    public List<Human> getAunts() {
        List<Human> siblings = new ArrayList<>();
        siblings.addAll(getSiblingsOfParents());

        List<Human> aunts = new ArrayList<Human>();
        for (Human sibling : siblings) {
            if (sibling.isFemale()) {
                aunts.add(sibling);
            }
        }

        return aunts; 
    }

    public Human getFirstFather() {
        Human currentFather = null;
        if (father != null) {
            currentFather = father;
        }
        while (currentFather.getFather() != null) {
            currentFather = currentFather.getFather();
        }
        return currentFather;
    }
}
\$\endgroup\$
5
\$\begingroup\$

thanks for sharing your code

Choose most simple interface

Your member children is of type Set but your getter/setter for this property require List.

Since the List interface guarantees order of entries while Set does not your users may get confusing result when putting in a list of cildren and getting it back in a different random order. This also forces you to transfer the elements from List to Set and vize versa, wich is a contradiction to tou performance premize.

The better approach would be to chang the member and the getter/setter to Collection which allows for both, List and Set implementations.

Performance

It was particularly important to me to program the class in high-performant[...]

When coding in a high level programmining language the basic rule is: first make it run, then make it fast!

Never choose a particular approach for (only) performance reasons if there is anothere being more readable. Esprecially in managed languages like Java, C# and alike following SOLID principles and clean code will usually lead to performant solutions.

Having blamed premarture optimization there are some points in your code that may affect performance.

The general rule with performance is The fastest way to do something is not doing it. And what hurts performance most is "hidden" iterations over lists or arrays. Such an "hidden iteration" is your List/Set conversion.

Structure your code

Especially when creating a lib to be used by others it should support the MVC - Pattern. In your case there should be a model class serving as dump data container and another class providing the business logic such as traversing the tree to find relatives.

code duplication

Your code has lots of duplicated code like this:

public List<Human> getSiblings() {
    Set<Human> siblings = new HashSet();
    if (father != null) {
        for (Human child : father.getChildren()) {
            if (child != this) {
                siblings.add(child);
            }
        }
    }
    if (mother != null) {
        for (Human child : mother.getChildren()) {
            if (child != this) {
                siblings.add(child);
            }
        }
    }
    // ...

This should be extracted to parameterized methods like this:

private Set<Human> getSiblingsOf(Human parent) {
    Set<Human> siblings = new HashSet();
    if (parent != null) {
        for (Human child : parent.getChildren()) {
            if (child != this) {
                siblings.add(child);
            }
        }
    }
    return siblings;
}

public List<Human> getSiblings() {
    Set<Human> siblings = new HashSet();
    siblings.addAll(getChildrenOf(father));
    siblings.addAll(getChildrenOf(mother));
    // ...

Comments

Comment public interface

Ad comments to your public method that explain how the caller should use this (not what is does).

Especially explain the parameters (whether they can be null or not) and the return value.

Avoid useless comments

Don't use comments to structure code

In your method getSiblingsOfParents() you "structure" your code into logical sections by inline comments. You should better extract such sections in methods with names derived from the comments.

Delete outdated comments

In the same method your comments are a good example why you should not use them: both comments are the same and you did not take care of changing it after copying. This is a common problem... ;o)

\$\endgroup\$
8
\$\begingroup\$

There have been quite a lot of comments about the incorrect simplification of gender and marital relations, so I will not rehash that in this answer. Instead let's take a slightly different approach to what you model.

Instead of encapsulating relationships between persons into the persons themselves (as mother, father, marriagePartner and children) how about we encapsulate relationships as a separate entity.

This allows us to change the relationship between two people without changing the persons themselves.

In the following I will assume there is a Person class encapsulating everything you wanted to know about a person. If we take the existing code at face value that's firstName, lastName and male. This is the moment where I tell you that everything you think you knew about names is wrong, but alas, we won't need names anymore.

There's something like two possible relationship types that are encoded in your program. The first is a "Parent-Child" relationship and the second is marriage. To make this somewhat more extensible, we might want to add further relationship types in the future. For now we encapsulate this into an enum.

enum RelationshipKind {
    ParentChild,
    Spouse
}

Thusly we get to a rather simple encoding of people and their relationships, namely a directed graph with named edges. Let's just finish specifying Relationships (or edges) and we're basically done:

@Getters
class Relationship {
    private RelationshipKind relationshipKind;
    private Person a;
    private Person b;
}

We can now model family trees with relative ease. This also allows us to encode divorces by adding a new relationship kind. A common way to keep this properly used is an adjacency matrix or an Adjacency list.

Since for large families this graph is rather sparse (little relationships for many people), an adjacency list is probably the fastest way to go about this:

public class Family {
    private final Map<Person, Set<Relationship>> directRelationships = new HashMap<>();

    public void marry(Person a, Person b) {
        Relationship marriage = new Relationship(RelationshipKind.Spouse, a, b);
        directRelationships.computeIfAbsent(a,  HashSet::new).add(marriage);
        directRelationships.computeIfAbsent(b,  HashSet::new).add(marriage);
    }

    public void bearChild(Person mother, Person child) {
        Relationship motherhood = new Relationship(ParentChild, mother, child);
        Optional<Relationship> otherParent = directRelationships.get(mother)
            .stream().filter(r -> r.getRelationshipKind() == Spouse)
            .findFirst()
            .map(marriage -> new Relationship(ParentChild, marriage.other(mother), child));

         directRelationships.computeIfAbsent(mother, HashSet::new).add(motherhood);
         directRelationships.computeIfAbsent(child, HashSet::new).add(motherhood);
         if (otherParent.isPresent()) {
            directRelationships.computeIfAbsent(otherParent.getA(), HashSet::new).add(otherParent);
            directRelatinships.get(child).add(otherParent);
         }
    }
}

How you implement Grandparents, Siblings, Uncles and Aunts as well as all other "family relations"... That is a matter of traversing the graph encapsulated in Family properly.

\$\endgroup\$
3
\$\begingroup\$

Avoid creating a new List just to copy values

Rather than creating a List, filling it and then copying selected items of that List into a new List, you may want to look into Java 8's Stream functionality (see Oracle's description of Streams).

public List<Human> getUncles() {
    List<Human> siblings = new ArrayList<>();
    siblings.addAll(getSiblingsOfParents());
    List<Human> uncles = new ArrayList<Human>();
    for (Human sibling : siblings) {
        if (sibling.isMale()) {
            uncles.add(sibling);
        }
    }
    return uncles; 
}

would become something like (untested)

public List<Human> getUncles() {
    List<Human> uncles =
        getSiblingsOfParents().stream()
            .filter(h -> h.isMale())
            .collect(toList());
    return uncles; 
}

Revise your tests

I have already tested this class by creating a test class that analyzes my own family tree.

.. but still the tests seem to have missed that people only have ever been successfully married to people, who are male and not male at the same time

public boolean setMarriagePartner(Human marriagePartner) {
    if (marriagePartner.isMale() != marriagePartner.isMale()) {
        ..
    }
    return false;
}

You may want to go over them again.. :-)

\$\endgroup\$
  • 1
    \$\begingroup\$ "Rather than creating a List, filling it and then copying selected items of that List into a new List, you may want to look into Java 8's Stream functionality" you do not avoid "creating a List, filling it", you just hide it behind the streams API which is sort of "hidden iteration" mentioned in my answer. \$\endgroup\$ – Timothy Truckle Jun 19 '18 at 17:05
  • \$\begingroup\$ @TimothyTruckle I completely agree with you on the hidden iteration issue. I mentioned Streams as a more elegant and readable way to do what OP has set out to do, using only one List rather than two, but I agree, leaving it out altogether would be even better. \$\endgroup\$ – glissi Jun 20 '18 at 7:35
  • 1
    \$\begingroup\$ "I mentioned Streams as a more elegant and readable way" I'd agree to that and I'd upvote your answer if that was its main statement. But your heading is implying that with the streams API no list object would be created and no iteration would occur. That is simply wrong. \$\endgroup\$ – Timothy Truckle Jun 20 '18 at 9:11
  • \$\begingroup\$ @TimothyTruckle in the original code, two new Lists are created (one with all the elements of the original Set, and one with only the relevant ones). In glissi's example, that first List would not be created. \$\endgroup\$ – Theoriok Jul 4 '18 at 15:48
3
\$\begingroup\$

While others have focused on the code, I'd like to address the underlying assumptions of the simplified human/family model presented here, resulting in a design that will fail to cover many real people and families. It's clear that this was discussed in comments, but I feel it's important to capture in an answer where it can persist (as Vogel612 suggested).

This list is not intended to be exhaustive, but here are some of the basic things that your code doesn't appear to handle.

For gender:

  • a person may have a gender other than male or female
  • a person's gender may not be constant throughout their life
  • a person's gender may not be one of the genders you are aware of at time of coding

For familial relationships:

  • partners
    • a person may have a significant non-marriage partner relationship, e.g. a civil union, or a long-term/lifetime partner without legal status
    • a person may have different partners over time due to divorce or death and remarriage
    • a person may have multiple partners simultaneously
  • parents/children
    • a person may have a child who is not their partner's child
    • a person may have different types of parents due to foster care and adoption
    • a person may have different parents over time due to partner relationship changes among parents
    • parental relationships can be ended, notably emancipation of minors
  • siblings
    • siblings may be step-siblings or half-siblings
    • people who share a parent may not be siblings at all in practice

And finally, any of these relationships may be of a variety you're not familiar with at time of coding.

Much of this dovetails nicely with Vogel612's suggestions: separating relationships from people, and the ability to add new relationship types to an enum, covers a great deal of the familial relationship complexities. You might additionally benefit from splitting it into PartnerKind, SiblingKind, and ParentKind, so that your code can be more explicitly aware of the difference between those overall categories of relationships.

Beyond that, much of your data would benefit from having associated time ranges. For example, a partner relationship has a start date and possibly an end date.

For gender, an enum is likely the best starting place, just as it was for relationship types. For both relationships and gender, it would also be useful to allow an arbitrary user-provided string, which would presumably then be associated with an enum value (even if it's PartnerOther) so that for example you would still know to display a custom "civil partnership" as a partnership.

I understand that this seems like a lot of complexity and corner cases to cover. But in this case, you're trying to model the real world, and the real world is complex - and all of these cases are important.

Some of these situations likely happen in almost any sufficiently broad family tree, e.g. close to 1/4 of married people in the U.S. have been previously married.

Some of them are less common, but to those who would be excluded from proper representation in your family tree, that will still feel like a pretty big problem, and by the time your code is used to cover reasonably large families - tens to hundreds of people - and used by many others, you're rapidly going to start encountering all of these situations.

\$\endgroup\$

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.