OdeToCode Refactoring Kata

Question

I saw this article that proposes a refactoring exercise and thought I'd give it a try.

Spoiler alert: I'm going to show my solution to the Kata below. You might want to attempt the Kata yourself before you look at this code, so this will not influence you in any way.

I ended up with the following three classes:

Person.cs

public class Person
{
    public string Name { get; set; }
    public DateTime BirthDate { get; set; }

    public bool IsOlderThan(Person person2)
    {
        return BirthDate > person2.BirthDate;
    }
}

Pair.cs

public class Pair
{
    public Person Person1 { get; set; }
    public Person Person2 { get; set; }
    public TimeSpan AgeDifference { get { return Person2.BirthDate - Person1.BirthDate; } }

    public Pair(Person person1, Person person2)
    {
        if (person1.IsOlderThan(person2))
        {
            Person1 = person2;
            Person2 = person1;
        }
        else
        {
            Person1 = person1;
            Person2 = person2;
        }
    }

    public Pair()
    {
        Person1 = Person2 = null;
    }
}

Finder.cs

public class Finder
{
    private readonly List<Person> _person;

    public Finder(List<Person> person)
    {
        _person = person;
    }

    public Pair FindClosestAgeInterval()
    {
        return Find(pairs => pairs.OrderBy(p => p.AgeDifference).FirstOrDefault());
    }

    public Pair FindFurthestAgeInterval()
    {
        return Find(pairs => pairs.OrderByDescending(p => p.AgeDifference).FirstOrDefault());
    }

    public Pair Find(Func<IEnumerable<Pair>, Pair> pairSelectionLambda)
    {
        var availableDistinctPairs = GetDistinctPairs();

        return pairSelectionLambda(availableDistinctPairs) ?? new Pair();
    }

    private IEnumerable<Pair> GetDistinctPairs()
    {
        for (var i = 0; i < _person.Count - 1; i++)
            for (var j = i + 1; j < _person.Count; j++)
                yield return new Pair(_person[i], _person[j]);
    }
}

The test class basically remained the same (except that I have updated it with the new class/method names).

Overall I think this code is OK (it passes the tests and I could easily understand it 6 months from now).

However, I'm interested to hear your critic opinions, to find out how it can get better.

Answer 1

My $0.02. Your solution is quite good as it stands. Just putting my thing down too.

I'm a fan of interface-based development with factory methods, so I extracted out an interface for the Person class called IPerson:

public interface IPerson
{
    string Name { get; }

    DateTime BirthDate { get; }
}

and then implemented Person as a sealed immutable class (using readonly and a parametrized private constructor) which has a factory method:

public sealed class Person : IPerson
{
    private readonly string name;

    private readonly DateTime birthDate;

    private Person(string name, DateTime birthDate)
    {
        this.name = name;
        this.birthDate = birthDate;
    }

    public string Name { get { return this.name; } }

    public DateTime BirthDate { get { return this.birthDate; } }

    public static IPerson Create(string name, DateTime birthDate)
    {
        return new Person(name, birthDate);
    }
}

I did similarly with the class you called Pair, but I called it PersonBirthdayDifference (I'll freely admit I can't think up a useful name at all). Also note the static Empty property which I put to use in the Finder algorithm rather than just newing up one with no properties set.

public interface IPersonBirthdayDifference
{
    IPerson YoungerPerson { get; }

    IPerson ElderPerson { get; }

    TimeSpan AgeDifference { get; }
}

and

public sealed class PersonBirthdayDifference : IPersonBirthdayDifference
{
    private static readonly IPersonBirthdayDifference empty = Create(null, null);

    private readonly IPerson youngerPerson;

    private readonly IPerson elderPerson;

    private readonly TimeSpan ageDifference;

    private PersonBirthdayDifference(IPerson youngerPerson, IPerson elderPerson)
    {
        this.youngerPerson = youngerPerson;
        this.elderPerson = elderPerson;
        this.ageDifference = (youngerPerson == null) || (elderPerson == null)
            ? default(TimeSpan)
            : elderPerson.BirthDate - youngerPerson.BirthDate;
    }

    public static IPersonBirthdayDifference Empty { get { return empty; } }

    public IPerson YoungerPerson { get { return this.youngerPerson; } }

    public IPerson ElderPerson { get { return this.elderPerson; } }

    public TimeSpan AgeDifference { get { return this.ageDifference; } }

    public static IPersonBirthdayDifference Create(IPerson p1, IPerson p2)
    {
        return new PersonBirthdayDifference(p1, p2);
    }
}

Finally, I update the Finder class by some of the same methods as above, plus introduce a couple more helper classes called GreaterThanComparer and LessThanComparer (not posting that code, it's rather trivial) to keep the switch from executing every iteration of the loop. There's also a healthy dose of LINQ to help declare intent where I can:

public sealed class Finder : IFinder
{
    private readonly IList<IPerson> people;

    private Finder(IEnumerable<IPerson> people)
    {
        this.people = people.ToList();
    }

    public static IFinder Create(IEnumerable<IPerson> people)
    {
        return new Finder(people);
    }

    public IPersonBirthdayDifference Find(FinderType finderType)
    {
        var peopleInOrder = this.PopulateListInOrder();

        return peopleInOrder.Any() ? GetAnswer(peopleInOrder, finderType) : PersonBirthdayDifference.Empty;
    }

    private static IPersonBirthdayDifference GetAnswer(
        IEnumerable<IPersonBirthdayDifference> peopleInOrder,
        FinderType finderType)
    {
        IPersonDifferenceComparer comparer;

        switch (finderType)
        {
            case FinderType.LessThan:
                comparer = LessThanComparer.Create();
                break;
            case FinderType.GreaterThan:
                comparer = GreaterThanComparer.Create();
                break;
            default:
                return PersonBirthdayDifference.Empty;
        }

        var answer = peopleInOrder.First();

        foreach (var person in peopleInOrder)
        {
            if (comparer.Compare(person.AgeDifference, answer.AgeDifference))
            {
                answer = person;
            }
        }

        return answer;
    }

    private IEnumerable<IPersonBirthdayDifference> PopulateListInOrder()
    {
        IList<IPersonBirthdayDifference> peopleInOrder = new List<IPersonBirthdayDifference>();

        for (var i = 0; i < this.people.Count - 1; i++)
        {
            for (var j = i + 1; j < this.people.Count; j++)
            {
                var isYounger = this.people[i].BirthDate < this.people[j].BirthDate;
                var youngerPerson = isYounger ? this.people[i] : this.people[j];
                var elderPerson = isYounger ? this.people[j] : this.people[i];

                peopleInOrder.Add(PersonBirthdayDifference.Create(youngerPerson, elderPerson));
            }
        }

        return peopleInOrder;
    }
}

Answer 2

Here's a stab at an extensible solution (I crammed everything into a single file to reduce the length). While some would argue this goes beyond the scope of the original requirements, it aims to provide extensibility points and keeps the Open-Close Principle (and DRY) in mind throughout. The client can either use an existing search algorithm (greatest or least age difference) or specify their own. Although both of these built-in solutions execute O(n) linear searches, the open interface would allow a client to provide a different / more efficient algorithm if they so desired.

This is certainly a worthwhile exercise, especially to anyone that doesn't fully realize the importance of writing legible code and proper Unit Tests. The real beauty of all that time spent writing tests is immediately apparent when you start modifying the internals and can confirm at a glance that the changes have not inadvertently broken some other part of the system. The need for legible code speaks for itself when you start with something as unbelievably cryptic as this sample.

using System;
using System.Collections.Generic;
using System.Linq;

namespace Algorithm
{
    /// <summary>
    /// Provides search capability over a given set of persons in order to select
    /// an appropriate pair of persons.
    /// </summary>
    public class PersonPairSearcher
    {
        private readonly IEnumerable<Person> persons;
        private readonly IEnumerable<PersonPair> pairs;

        public PersonPairSearcher(List<Person> persons)
        {
            this.persons = persons;
            this.pairs = new PersonPairGenerator(persons).Pairs;
        }

        /// <summary>
        /// Locates the best matching pair in the given list of people
        /// based upon the specified search criteria.
        /// </summary>
        /// <param name="mode">Search algorithm to be used to locate the best matching pair.</param>
        /// <returns></returns>
        public PersonPair Find(IPersonSearchBehavior searchBehavior)
        {
            if (!pairs.Any())
                return PersonPair.Empty;

            return searchBehavior.GetMatch(pairs);
        }
    }

    /// <summary>
    /// Represents a pair of Persons.
    /// </summary>
    public class PersonPair
    {
        public static PersonPair Empty { get { return new PersonPair(null, null); } }

        public PersonPair(Person person1, Person person2)
        {
            //a pair can't have only one person.
            if (person1 == null || person2 == null)
                return;

            if (person1.BirthDate > person2.BirthDate)
            {
                YoungerPerson = person2;
                OlderPerson = person1;
            }
            else
            {
                YoungerPerson = person1;
                OlderPerson = person2;
            }
        }

        public Person YoungerPerson { get; private set; }
        public Person OlderPerson { get; private set; }
        public TimeSpan AgeDifference { get { return OlderPerson.BirthDate - YoungerPerson.BirthDate; } }
    }

    /// <summary>
    /// Represents a Person.
    /// </summary>
    public class Person
    {
        public string Name { get; set; }
        public DateTime BirthDate { get; set; }
    }

    /// <summary>
    /// Generates a set of pairs from the given set of persons.
    /// </summary>
    internal class PersonPairGenerator
    {
        private IEnumerable<Person> persons;
        private IEnumerable<PersonPair> pairs;

        public PersonPairGenerator(IEnumerable<Person> persons)
        {
            this.persons = persons;
            BuildPairs();
        }

        public IEnumerable<PersonPair> Pairs
        {
            get
            {
                return this.pairs;
            }
        }

        private void BuildPairs()
        {
            var pairs = new List<PersonPair>();

            for (var i = 0; i < persons.Count() - 1; i++)
            {
                for (var j = i + 1; j < persons.Count(); j++)
                {
                    var pair = new PersonPair(persons.ElementAt(i), persons.ElementAt(j));
                    pairs.Add(pair);
                }
            }

            this.pairs = pairs;
        }
    }

    /// <summary>
    /// A contract for a search algorithm that selects the best matching pair from the available set.
    /// </summary>
    public interface IPersonSearchBehavior
    {
        PersonPair GetMatch(IEnumerable<PersonPair> pairs);
    }

    /// <summary>
    /// A person pair searcher that locates the pair with the greatest difference in age.
    /// </summary>
    public sealed class LargestAgeDifferenceSearch : LinearSearchBehavior
    {
        protected override bool SuperseedsExistingMatch(PersonPair candidate, PersonPair existing)
        {
            return candidate.AgeDifference > existing.AgeDifference;
        }
    }

    /// <summary>
    /// A person pair searcher that locates the pair with the smallest difference in age.
    /// </summary>
    public sealed class SmallestAgeDifferenceSearch : LinearSearchBehavior
    {
        protected override bool SuperseedsExistingMatch(PersonPair candidate, PersonPair existing)
        {
            return candidate.AgeDifference < existing.AgeDifference;
        }
    }

    /// <summary>
    /// Searches the given set of pairs linearly, checking if each is a better match than the last.
    /// </summary>
    public abstract class LinearSearchBehavior : IPersonSearchBehavior
    {
        public PersonPair GetMatch(IEnumerable<PersonPair> pairs)
        {
            if (!pairs.Any())
                return null;

            var result = pairs.ElementAt(0);
            for (int i = 1; i < pairs.Count(); i++)
            {
                var candidate = pairs.ElementAt(i);
                if (SuperseedsExistingMatch(candidate, result))
                    result = candidate;
            }

            return result;
        }

        protected abstract bool SuperseedsExistingMatch(PersonPair candidate, PersonPair existing);
    }
}