Geektrust: Modelling Family Relationships in a OO way

Question

I'm trying to model family relationships in a OO way, i.e. As we update the relationships between two individuals our model must be able to find the people given a person and the type of relationship

• Given a ‘name’ and a ‘relationship’, you should output the people corresponding to the relationship in the order in which they were added to the family tree.

• You should be able to add a child to any family in the tree through the mother.

Sample input 
ADD_CHILD Chitra Aria Female
GET_RELATIONSHIP Lavnya Maternal-Aunt
GET_RELATIONSHIP Aria Siblings

Sample Output
CHILD_ADDITION_SUCCEEDED
Aria
Jnki Ahit

Full text of the problem here

My approach is mostly from this SO question.

• The relationship between two individuals is maintained as a graph using the adjacency list representation in the FamilyGraph class.
• PersonStore acts as a repository of the persons involved.
• BaseRelationships is a static class which provides extension methods on the Person class for the intrinsic relationships that can be used to build to find any sort of relationships.
• Relationships is the public facing API which has all the relations that our model can currently figure out.

Person class

namespace Family.Implementation
{
    public class PersonStore : IPersonStore
    {
        private Dictionary<string, Person> people;

        public PersonStore()
        {
            people = new Dictionary<string, Person>();
        }

        public void Add(Person person)
        {
            if(Contains(person.Name))
            {
                throw new ArgumentException($"{person.Name} is already present");
            }
            people.Add(person.Name, person);
        }

        public void Add(IEnumerable<Person> people)
        {
            foreach (var person in people)
            {
                try
                {
                    Add(person);
                }
                catch (Exception)
                {
                    throw;
                }
            }
        }

        public bool Contains(string personName)
        {
            return people.ContainsKey(personName);
        }

        public IEnumerable<Person> GetPeople(List<string> people)
        {
            List<Person> result = new List<Person>();
            people.ForEach(person => {
                try
                {
                    Person personObject = GetPerson(person);
                    result.Add(personObject);
                }
                catch (Exception)
                {
                    throw;
                }
            });
            return result;
        }

        public Person GetPerson(string personName)
        {
            Person person;
            bool result = people.TryGetValue(personName, out person);
            if(!result)
            {
                throw new ArgumentException($"{personName} isn't found");
            }
            return person;
        }
    }
}

FamilyGraph class

namespace Family.Implementation
{
    public class FamilyGraph : IFamilyGraph
    {
        private Dictionary<Person, PersonRelationships> Families;
        public FamilyGraph()
        {
            Families = new Dictionary<Person, PersonRelationships>();
        }
        public IPersonStore PersonStore { get { return ServiceLocator.GetService<IPersonStore>(); } }

        public void Add(EdgeInput inputEdge)
        {
            Edge edge;
            try
            {
                edge = GetEdge(inputEdge);
            }
            catch (ArgumentException)
            {
                throw;
            }
            switch (edge.RelationshipType)
            {
                case Enums.RelationshipType.Parent:
                    AddParentRelationship(edge);
                    return;
                case Enums.RelationshipType.Spouse:
                    AddSpouseRelationship(edge);
                    return;
            }
        }
        private void AddSpouseRelationship(Edge edge)
        {
            PersonRelationships sourcePersonRelationships, targetPersonRelationships;
            Families.TryAdd(edge.Source, new PersonRelationships());
            Families.TryAdd(edge.Target, new PersonRelationships());

            Families.TryGetValue(edge.Source, out sourcePersonRelationships);
            Families.TryGetValue(edge.Target, out targetPersonRelationships);

            if (targetPersonRelationships.Spouse == null && sourcePersonRelationships.Spouse == null)
            {
                targetPersonRelationships.AddSpouse(edge.Source);
                sourcePersonRelationships.AddSpouse(edge.Target);
            }
            else
            {
                throw new InvalidOperationException($"Cannot add spouse");
            }
        }
        private void AddParentRelationship(Edge edge)
        {
            PersonRelationships sourcePersonRelationships, targetPersonRelationships;
            Families.TryAdd(edge.Source, new PersonRelationships());
            Families.TryAdd(edge.Target, new PersonRelationships());

            Families.TryGetValue(edge.Source, out sourcePersonRelationships);
            Families.TryGetValue(edge.Target, out targetPersonRelationships);
            if (targetPersonRelationships.CanAddParent(edge.Source))
            {
                targetPersonRelationships.AddParent(edge.Source);
                sourcePersonRelationships.AddEdge(edge);
            }
            else
            {
                throw new InvalidOperationException($"Cannot add parents to {edge.Target.Name}");
            }
        }
        public Edge GetEdge(EdgeInput inputEdge)
        {
            Person source, target;
            try
            {
                source = PersonStore.GetPerson(inputEdge.Source);
                target = PersonStore.GetPerson(inputEdge.Target);
            }
            catch (Exception)
            {

                throw;
            }
            return new Edge(source, target, inputEdge.RelationshipType);
        }
        public IPersonRelationships Get(Person person)
        {
            PersonRelationships personRelationships;
            Families.TryGetValue(person, out personRelationships);
            return personRelationships;
        }
    }
}

and the related DTO's PersonRelationship, Person and EdgeInput

namespace Family.Implementation
{
    public sealed class PersonRelationships : IPersonRelationships
    {
        public List<Edge> Edges { get; private set; }
        public List<Person> Parents { get; private set; }
        public Person Spouse { get; private set; }

        public PersonRelationships()
        {
            Edges = new List<Edge>();
            Parents = new List<Person>();
        }
        public PersonRelationships(List<Edge> edges, List<Person> persons)
        {
            Edges = edges;
            Parents = persons;
        }

        public bool CanAddParent(Person parent)
        {
            if (Parents.Count() == 2)
                return false;
            return !Parents.Any(m => m.Gender == parent.Gender);
        }

        public void AddEdge(Edge edge)
        {
            Edges.Add(edge);
        }

        public void AddParent(Person parent)
        {
            Parents.Add(parent);
        }

        public void AddSpouse(Person spouse)
        {
            Spouse = spouse;
        }
    }
}

namespace Family.DTO
{
    public class Edge
    {
        public Edge(Person source, Person target, RelationshipType relationshipType)
        {
            Source = source ?? throw new ArgumentNullException(nameof(source));
            Target = target ?? throw new ArgumentNullException(nameof(target));
            RelationshipType = relationshipType;
        }

        public Person Source { get; }
        public Person Target { get; }
        public RelationshipType RelationshipType { get; }
    }
}

namespace Family.DTO
{
    public class Person
    {
        public Person(string name, Gender gender)
        {
            if (string.IsNullOrWhiteSpace(name))
            {
                throw new ArgumentException("message", nameof(name));
            }

            Name = name;
            Gender = gender;
        }

        public string Name { get; }
        public Gender Gender { get; }
    }
}

enum RelationshipType

namespace Family.Enums
{
    public enum RelationshipType
    {
        Parent,
        Spouse
    }
}

and the BaseRelationships that exposes the intrinsic functions as extention methods.

namespace Family.Implementation
{
    public static class BaseRelationships 
    {
        public static IFamilyGraph FamilyGraph = ServiceLocator.GetService<IFamilyGraph>();
        public static IEnumerable<Person> Parents(this IEnumerable<Person> people, Gender? gender = null)
        {
            return people.SelectMany(m => m.Parents());
        }
        public static IEnumerable<Person> Children(this IEnumerable<Person> people, Gender? gender = null)
        {
            return people.SelectMany(m => m.Children(gender));
        }
        public static IEnumerable<Person> Siblings(this IEnumerable<Person> people, Gender? gender = null)
        {
            return people.SelectMany(m => m.Siblings(gender));
        }
        public static IEnumerable<Person> Spouse(this IEnumerable<Person> people)
        {
            return people.SelectMany(m => m.Spouse());
        }

        public static IEnumerable<Person> Parents(this Person person, Gender? gender = null)
        {
            List<Person> result = new List<Person>();
            IPersonRelationships personRelationships = FamilyGraph.Get(person);
            if(personRelationships == null)
            {
                return result;
            }
            IEnumerable<Person> parents = personRelationships.Parents
                .Where(m => gender == null || m.Gender == gender);
            result.AddRange(parents);
            return result;
        }
        public static IEnumerable<Person> Children(this Person person, Gender? gender = null)
        {
            List<Person> result = new List<Person>();
            IPersonRelationships personRelationships = FamilyGraph.Get(person);
            List<Person> children = personRelationships.Edges
                .Where(m => m.RelationshipType == RelationshipType.Parent)
                .Where(m=> gender == null || m.Target.Gender == gender)
                .Select(m => m.Target)
                .ToList();
            result.AddRange(children);
            return result;
        }
        public static IEnumerable<Person> Siblings(this Person person, Gender? gender = null)
        {
            return person.Parents()
                .Children(gender)
                .Distinct()
                .Where(m => !m.Equals(person));
        }
        public static IEnumerable<Person> Spouse(this Person person)
        {
            List<Person> result = new List<Person>();
            IPersonRelationships personRelationships = FamilyGraph.Get(person);
            if (personRelationships.Spouse != null)
            {
                result.Add(personRelationships.Spouse);
            }
            return result;
        }
    }
}

and the public facing API

namespace Family.Implementation
{
    public sealed class Relationships : IRelationships
    {
        private IPersonStore PersonStore;
        public Relationships()
        {
            var personStore = ServiceLocator.GetService<IPersonStore>();
            PersonStore = personStore ?? throw new Exception("Could not load dependencies");
        }
        public Person GetPerson(string person)
        {
            Person personObject;
            try
            {
                personObject = PersonStore.GetPerson(person);
            }
            catch (Exception)
            {
                throw;
            }
            return personObject;
        }

        public IEnumerable<Person> BrotherInLaw(string person)
        {
            return InLaws(person, Gender.Male);
        }

        public IEnumerable<Person> Daughter(string person)
        {
            return Children(person, Gender.Female);
        }

        public IEnumerable<Person> MaternalAunt(string person)
        {
            return UncleAndAunt(person, "Maternal", "Aunt");
        }

        public IEnumerable<Person> MaternalUncle(string person)
        {
            return UncleAndAunt(person, "Maternal", "Uncle");
        }

        public IEnumerable<Person> PaternalAunt(string person)
        {
            return UncleAndAunt(person, "Paternal", "Aunt");
        }

        public IEnumerable<Person> PaternalUncle(string person)
        {
            return UncleAndAunt(person, "Paternal", "Uncle");
        }

        public IEnumerable<Person> Siblings(string person)
        {
            Person personObject;
            try
            {
                personObject = GetPerson(person);
            }
            catch (Exception)
            {
                throw;
            }
            return personObject.Siblings();
        }

        public IEnumerable<Person> SisterInLaw(string person)
        {
            return InLaws(person, Gender.Female);
        }

        public IEnumerable<Person> Son(string person)
        {
            return Children(person, Gender.Male);
        }

        private IEnumerable<Person> UncleAndAunt(string person, string direction, string uncleOrAunt)
        {
            Person personObject;
            try
            {
                personObject = GetPerson(person);
            }
            catch (Exception)
            {
                throw;
            }
            Gender parentsGender = direction == "Maternal" ? Gender.Female : Gender.Male;
            Gender uncleOrAuntGender = uncleOrAunt == "Aunt" ? Gender.Female : Gender.Male;
            return personObject.Parents(parentsGender)
                .Siblings(uncleOrAuntGender);
        }

        private IEnumerable<Person> Children(string person, Gender gender)
        {
            Person personObject;
            try
            {
                personObject = GetPerson(person);
            }
            catch (Exception)
            {
                throw;
            }
            return personObject.Children(gender);
        }

        private IEnumerable<Person> InLaws(string person, Gender gender)
        {
            Person personObject;
            try
            {
                personObject = GetPerson(person);
            }
            catch (Exception)
            {
                throw;
            }
            return personObject.Spouse().Siblings(gender);
        }
    }
}

• I user service locator instead of DI as I have to inject the FamilyGraph dependency on the BaseExtentions static class to make them as extensions methods. Which unfortunately made testing really hard.
• I believe extending the four intrinsic functions as extension methods make writing further relationships more intuitive and understandable

It would be great if you can comment on

• If the object composition is accurate enough.
• way to better inject FamilyGraph and yet have them as extension methods to the Person Object

Note I have ignored the IO part to call these relationships and also add the child for the review.

The entire code is also available on GitHub to make it easier to read.

Answer 1

Design

• As you noticed, a service locator is a problematic solution for your IFamilyGraph dependency problem in BaseRelationships. There's a much simpler solution: make these methods extensions for IFamilyGraph. It's IFamilyGraph that lets you obtain relationships for a person, so that is the API that you're extending. Alternately, you could pass IFamilyGraph as an argument, but that results in a less intuitive API in my opinion.
• Supporting 'intrinsic' relationships via extension methods is not necessarily a good idea. For example, because you're limited to using a public API, you may not be able to apply certain optimizations compared to a 'native' implementation. In this case, you're fetching all relationships of a person, only to select a few of them, or to fetch the relationships of those relationships. With a database-backed implementation, this may lead to more database roundtrips and more data transfer than necessary.
• Your naming is inconsistent. Edge make sense when dealing with a graph data structure, but because your API is focused on family relationships, and because you're referring to Persons rather than nodes, I would rename Edge to Relationship. Similarly, Add and Get make sense in a general-purpose data structure, but here names like AddPerson and GetRelationships improve clarity.
• Splitting things up into an IPersonStore and IFamilyGraph might make sense, but having to use both at different times makes your code difficult to use. I would combine them into a single API that lets you register and look up both people and their relationships.

Other notes

• FamilyGraph depends on an IPersonStore, so I would pass it as a constructor argument. This makes the dependency visible, and gives you more flexibility, such as being able to simultaneously test multiple family graphs, each with their own test person store.
• FamilyGraph uses Person as key. Person does not implement Equals and GetHashCode, so that is problematic, but this also means that FamilyGraph 'duplicates' data from PersonStore, and that callers are forced to first obtain a Person object before they can fetch that persons relationships. That complicates things - see Relationships. Using person names (or rather, a unique identifier) for lookup and as keys whenever possible should make the API a little easier to use.
• With FamilyGraph.PersonStore being injected, the relationship extension methods being extensions on IFamilyGraph, and FamilyGraph.Get no longer requiring a Person but a name instead, there's no need for ServiceLocator anymore.
• FamilyGraph.Get returns a mutable relationships object. This allows outside code to mess with internal state, bypassing FamilyGraph.Add. With the current implementation, that may work, but it'll break when you switch to a database-backed graph. Don't expose internal state: return a sequence of immutable relationships instead.
• There's no use for IPersonRelationships and IRelationships - PersonRelationships is a simple internal data object, and Relationships is a set of high-level convenience methods. There's no use in creating interfaces for everything.
• Organizing code into namespaces based on type (enums, interfaces) is not very useful, I think. In this case there aren't that many types anyway, so you might as well leave everything in the same namespace. Mark types that should not be exposed as part of the public API as internal.
• It's a good idea to document preconditions (such as that PersonStore.Add(Person) won't accept a person whose name is already taken) and failure behavior (such as the effect of PersonStore.Add(IEnumerable<Person>) when one of the given persons cannot be added - does it roll back the changes? If not, how does a caller know which persons were added?).

C# specific

• Using field initializers lets you remove the PersonStore and FamilyGraph constructors.
• try { DoSomething(); } catch (Exception) { throw; } is pointless. Just write DoSomething(); instead.
• You could make more use of var. There's no need to repeat type names in statements like List<Person> result = new List<Person>();.
• Accepting a List<T> instead of an IEnumerable<T> so you can use List.ForEach is restrictive for callers, and doesn't really offer any benefits: ForEach can easily be replaced with a foreach loop.
• out variables can be declared in-line: people.TryGetValue(personName, out var person).
• As mentioned above, Person does not implement Equals and GetHashCode. This means that dictionary lookups will only work for the exact same instance: Families[new Person("A", Gender.Male)] = ...; Families.ContainsKey(new Person("A", Gender.Male)) --> false.
• Instead of doing a dictionary TryAdd method followed by a TryGetValue, call TryGetValue first and then, only if necessary, call Add.
• You occasionally create a list with ToList(), only to add it to another list (results.AddRange(...)) and then return that list. It's more efficient to return that first list directly.

Answer 2

Design

Your API allows consumers to navigate people by their relationships to other people. For this reason, it surprises me you define a person as a simple DTO. By making this decision, you require static classes to store the relationships and fishy extension methods to get the relationships.

 public static IEnumerable<Person> Children(this Person person, Gender? gender = null)
 {
     List<Person> result = new List<Person>();
     IPersonRelationships personRelationships = FamilyGraph.Get(person); // <- code-smell
     // .. 
 }

You could make a much cleaner API by using best practices when dealing with family trees. Person should not be a DTO, but a node instead. This allows navigation amongst people without having to require static dependencies.

public class Person
{
    public Person Father { get; private set; }
    public Person Mother { get; private set; }
    public Person Spouse { get; private set; }
    public IReadonlyCollection<Person> Children { get { /* .. */ }}

    // constructor, properties, and methods ..
}

Navigating the family tree becomes much simpler.

var grandMotherOfSpouseOfFatherOfAria = Aria.Father.Spouse.Mother.Mother;

Classes like Edge and Relationships have no added value for the consumer of the API. If you should decide to keep using them, make them private to the API.