Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
1  
I would rename the fields in Pair to reflect their respective ages. Maybe something like Senior and Junior or Elder and Younger. –  Carl Manaster Apr 9 '12 at 15:37
    
@CarlManaster you are right; the names you propose are very suggestive. I really shouldn't have left those property names as I did. –  w0lf Apr 9 '12 at 19:35

2 Answers 2

up vote 1 down vote accepted

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;
    }
}
share|improve this answer
1  
Re: specific to the Person class. I see this pattern a lot and like that it is immutable. Contrary to this approach, if I was to put on my devil's advocate hat, I'd be curious to your thoughts on this presentation about "Stop writing classes". In it, Jack Diederich claims that if you have a class with two methods and one of those is a [constructor], it shouldn't be a class. –  Ryan Miller Apr 9 '12 at 18:12
1  
I'm still watching the video, but he makes sense with his hypothesis of favor simplicity over complexity. This is a laudable goal, but I think it should follow the computer programming maxim of "it depends". It depends if you know this little complex beeble bobble will be needed down the road or it allows for better integration with unit testing, etc. I don't know if C# has a good analogue to the function construct he refers to in Python that would help accomplish that goal. I also think the dynamic nature of Python allows for some natural code shrinkage. –  Jesse C. Slicer Apr 9 '12 at 18:39
    
@JesseC.Slicer Thanks for sharing these enhacenments. I think that's really a good example for using the Null Object pattern; I like that you handled the case of one or both persons being null when calculating the Age (which I inexcusably forgot) and that the classes are immutable. I also think that your use of comparers can be more readable than the lambdas in my example. In this scenario, however, I think I'd go without extracting interfaces for the classes, unless I really have to for some reason. –  w0lf Apr 9 '12 at 19:48
    
@RyanMiller Thanks for sharing this interesting video. Jack's the examples are expressive and I perfectly agree with the idea of keeping things simple, but I also think he employs some extreme arguments in order to make his point. For example, encapsulation, decoupling and separation of concerns are not jokes, but concepts that actually yield great benefits when correctly applied. I agree with Jesse's "it depends". –  w0lf Apr 9 '12 at 20:04

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);
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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