3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would rename the fields in Pair to reflect their respective ages. Maybe something like Senior and Junior or Elder and Younger. \$\endgroup\$ – Carl Manaster Apr 9 '12 at 15:37
  • \$\begingroup\$ @CarlManaster you are right; the names you propose are very suggestive. I really shouldn't have left those property names as I did. \$\endgroup\$ – Cristian Lupascu Apr 9 '12 at 19:35
2
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Ryan Miller Apr 9 '12 at 18:12
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Jesse C. Slicer Apr 9 '12 at 18:39
  • \$\begingroup\$ @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. \$\endgroup\$ – Cristian Lupascu Apr 9 '12 at 19:48
  • \$\begingroup\$ @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". \$\endgroup\$ – Cristian Lupascu Apr 9 '12 at 20:04
  • \$\begingroup\$ I've seen static creates like this before.. why not In your person class just remove the create and make the ctor public, you can still get immutability? \$\endgroup\$ – CSharper Jun 1 '16 at 16:45
1
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ – Toby Speight Feb 11 at 10:20
0
\$\begingroup\$

I found this interesting and I did the job as well

I fully believe on the solid principles and object oriented principles, that's how I ended up with the following code:

The idea behind IFindStrategy interface is to make the solution extensible. That's why I have created it and my strategies inherit from it (See Strategy pattern)

IFindStrategy is the interface from which all my strategies will inherit

public interface IFindStrategy
{
    PeopleTuple Execute();
}

I have created an abstract class for my strategies so that the logic inside LessThan2People is not spread throughout the solution. We can also think about extending this abstract class in the feature (if needed). This is handy if we want to add behaviour to all our strategies from one place.

public abstract class FindStrategyBase
{
    protected readonly List<Person> Persons;

    protected FindStrategyBase(List<Person> persons)
    {
        Persons = persons;
    }

    public bool LessThan2People()
    {
        return Persons.Count < 2;
    }
}

FindStrategyBase is our based class and we inherited because we need the behaviour in that class inside our strategy.

IFindStrategy makes possible to pass millions of different strategies in our finder class without changing the code of our finder class, this is the beauty of the Open/Close principle. This is possible thanks to Thanks to polymorphism

FurthestTwoPeopleByAgeStrategy is one of my strategies, it inherits from FindStrategyBase and IFindStrategy. It looks for the Furthest two people inside the list persons base on their age.

public class FurthestTwoPeopleByAgeStrategy : FindStrategyBase, IFindStrategy
{
    public FurthestTwoPeopleByAgeStrategy(List<Person> persons) : base(persons)
    {
    }

    public PeopleTuple Execute()
    {
        if (LessThan2People())
            return PeopleTuple.None;

        var tr = new List<PeopleTuple>();

        for (var i = 0; i < Persons.Count - 1; i++)
        {
            for (var j = i + 1; j < Persons.Count; j++)
            {
                var r = new PeopleTuple(Persons[i], Persons[j]);
                tr.Add(r);
            }
        }

        return tr.Count == 1 ? tr[0] : tr.Aggregate((agg, next) =>
            next.DifferenceOfAges > agg.DifferenceOfAges ? next : agg);
    }
}

ClosestTwoPeopleByAgeStrategy is one of my strategies, it inherits from FindStrategyBase and IFindStrategy. It looks for the closest two people inside the list persons base on their age.

public class ClosestTwoPeopleByAgeStrategy : FindStrategyBase, IFindStrategy
{
    public ClosestTwoPeopleByAgeStrategy(List<Person> persons) : base(persons)
    {
    }

    public PeopleTuple Execute()
    {
        if (LessThan2People())
            return PeopleTuple.None;

        var tr = new List<PeopleTuple>();

        for (var i = 0; i < Persons.Count - 1; i++)
        {
            for (var j = i + 1; j < Persons.Count; j++)
            {
                var r = new PeopleTuple(Persons[i], Persons[j]);
                tr.Add(r);
            }
        }

        return tr.Count == 1 ? tr[0] : tr.Aggregate((agg, next) =>
            next.DifferenceOfAges < agg.DifferenceOfAges ? next : agg);
    }
}

Finder receives any strategy and executes it. Single Responsibility principle.

public class Finder
{
    private readonly IFindStrategy _findStrategy;

    public Finder(IFindStrategy findStrategy)
    {
        _findStrategy = findStrategy;
    }

    public PeopleTuple Find()
    {   
        return _findStrategy.Execute();
    }
}

Person class complies with encapsulation Object Oriented Principle. It forbids the access to its properties and provides only methods that are needed for the client.

public class Person : IEquatable<Person>
{
    private readonly string _name;
    private readonly DateTime _birthDate;

    public Person(string name, DateTime birthDate)
    {
        _name = name;
        _birthDate = birthDate;
    }

    public bool IsOlderThan(Person p) => _birthDate > p._birthDate;

    public TimeSpan DifferenceOfAges(Person p) => _birthDate - p._birthDate;

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != GetType()) return false;
        return Equals((Person) obj);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return ((_name != null ? _name.GetHashCode() : 0) * 397) ^ _birthDate.GetHashCode();
        }
    }

    public bool Equals(Person other)
    {
        if (ReferenceEquals(null, other)) return false;
        if (ReferenceEquals(this, other)) return true;
        return string.Equals(_name, other._name) && _birthDate.Equals(other._birthDate);
    }
}

PeopleTuple class complies with encapsulation Object Oriented Principle. It forbids the access to its properties and provides only methods that are needed for the client.

public class PeopleTuple
{
    public static PeopleTuple None => null;

    private Person YoungerPerson { get; set; }
    private Person OlderPerson { get; set; }
    public TimeSpan DifferenceOfAges => OlderPerson.DifferenceOfAges(YoungerPerson);

    public PeopleTuple(Person p1, Person p2)
    {
        OlderPerson = p1.IsOlderThan(p2) ? p1 : p2;
        YoungerPerson = p1.IsOlderThan(p2) ? p2 : p1;
    }

    public bool IsEqualsToYoungerPerson(Person p)
    {
        return YoungerPerson.Equals(p);
    }

    public bool IsEqualsToOlderPerson(Person p)
    {
        return OlderPerson.Equals(p);
    }
}

I have also changed the unit tests to reflect my changes

public class FinderTests
{
    [Fact]
    public void Returns_Empty_Results_When_Given_Empty_List()
    {
        var list = new List<Person>();
        var finder = new Finder(new ClosestTwoPeopleByAgeStrategy(list));
        var result = finder.Find();

        Assert.True(result == PeopleTuple.None);
    }

    [Fact]
    public void Returns_Empty_Results_When_Given_One_Person()
    {
        var list = new List<Person>() { sue };
        var finder = new Finder(new ClosestTwoPeopleByAgeStrategy(list));
        var result = finder.Find();

        Assert.True(result == PeopleTuple.None);
    }

    [Fact]
    public void Returns_Closest_Two_For_Two_People()
    {
        var list = new List<Person>() { sue, greg };
        var finder = new Finder(new ClosestTwoPeopleByAgeStrategy(list));
        var result = finder.Find();

        Assert.True(result.IsEqualsToYoungerPerson(sue));
        Assert.True(result.IsEqualsToOlderPerson(greg));
    }

    [Fact]
    public void Returns_Furthest_Two_For_Two_People()
    {
        var list = new List<Person>() { greg, mike };
        var finder = new Finder(new FurthestTwoPeopleByAgeStrategy(list));
        var result = finder.Find();

        Assert.True(result.IsEqualsToYoungerPerson(greg));
        Assert.True(result.IsEqualsToOlderPerson(mike));
    }

    [Fact]
    public void Returns_Furthest_Two_For_Four_People()
    {
        var list = new List<Person>() { greg, mike, sarah, sue };
        var finder = new Finder(new FurthestTwoPeopleByAgeStrategy(list));
        var result = finder.Find();

        Assert.True(result.IsEqualsToYoungerPerson(sue));
        Assert.True(result.IsEqualsToOlderPerson(sarah));
    }

    [Fact]
    public void Returns_Closest_Two_For_Four_People()
    {
        var list = new List<Person>() { greg, mike, sarah, sue };
        var finder = new Finder(new ClosestTwoPeopleByAgeStrategy(list));
        var result = finder.Find();

        Assert.True(result.IsEqualsToYoungerPerson(sue));
        Assert.True(result.IsEqualsToOlderPerson(greg));
    }

    Person sue = new Person("Sue", new DateTime(1950, 1, 1));
    Person greg = new Person("Greg", new DateTime(1952, 6, 1));
    private Person sarah = new Person("Sarah", new DateTime(1982, 1, 1));
    private Person mike = new Person("Mike", new DateTime(1979, 1, 1));
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ – Toby Speight Feb 11 at 10:20

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.