Name-deduplicating class

Question

Recently I asked a question on Programming SE about public vs private methods fo unit testing, which generated a lot of interesting responses.

I only posted a skeleton of the code there, as it was a question about structure and not about the code itself. However, several respondants queried whether my code might be overly complex, violate the SRP and that it might be "degenerate" because it only needed a single public method.

It seemed like too good a learning opportunity to miss, so I thought I'd post it here for review, with particular attention to above points and the structure and testability of the class.

I found the idea that it was poor design to have just one public method particularly interesting. On reflection, while it's clearly odd to have a class with just one public method, it seems to me that a class which keeps within the SRP probably ought to have very few.

Some notes on the external dependencies of the code. FileContents is just a DTO, a way of keeping useful properties together - in this instance a file that's read into FileRow objects and a Mappings collection that indicates which fields contain which data. ModelHelper contains some methods to work with these DTOs. JaroWinkler is a static class that implements that well known text similarity algorithm - it's pretty much the same as this.

public static class DeduplicateIndividuals
{
     public static void DedupeFile(FileContents fc)
    {
        BuildNameKeys(fc);
        SetExactDuplicates(fc);
        FuzzyMatching(fc);
    }

    private static bool SurnameMatch(string surname1, string surname2)
    {
        return JaroWinkler.Proximity(surname1, surname2) >= 0.75;
    }

    private static bool ForenameMatch(string forename1, string forename2)
    {
        if(forename1.FirstCharacter() == forename2.FirstCharacter())
        {
            return true;
        }

        return JaroWinkler.Proximity(forename1, forename2) >= 0.85;
    }

    private static bool TitleMatch(string title1, string title2)
    {
        if(MrAndMrs(title1, title2) || MrAndMrs(title2, title1))
        {
            return false;
        }

        return JaroWinkler.Proximity(title1, title2) >= 0.9;
    }

    // without this, JW can think that Mr is the same as Ms, Mrs and Miss
    private static bool MrAndMrs(string title1, string title2)
    {
        return title1 == "Mr" && (title2 == "Mrs" || title2 == "Ms" || title2 == "Miss");
    }

    public static void BuildNameKeys(FileContents fc)
    {
        int? dobOrdinal = ModelHelper.GetOrdinalByAssignedLabel(fc.Mappings, "DOB");

        foreach (FileRow fr in fc.FileRows)
        {
            fr.CleanedName = string.Format("{0} {1} {2}", fr.GetFieldByOrdinal(fc.TitleOrdinal),
                                    RemoveMiddleFromForename(fr.GetFieldByOrdinal(fc.ForenameOrdinal)), fr.GetFieldByOrdinal(fc.SurnameOrdinal));

            if (dobOrdinal != null)
            {
                fr.CleanedName += " " + fr.Fields[(int)dobOrdinal];
            }

            if(fr.CleanedAddress == "")
            {
                fr.CleanedAddress = ModelHelper.GetAddressFromMappings(fc, fr).ToString();
            }
        }
    }

    private static void SetExactDuplicates(FileContents fc)
    {
        var hashSet = new HashSet<string>();
        foreach (FileRow fr in fc.FileRows)
        {
            if (!hashSet.Add(fr.CleanedName + fr.CleanedAddress))
            {
                fr.PossibleDuplicate = true;
            }
        }
    }

    private static void FuzzyMatching(FileContents fc, int maxParallels = 32)
    {
        List<FileRow> copiedRows = new List<FileRow>(fc.FileRows.Where(f => f.PossibleDuplicate == false));

        Parallel.ForEach(fc.FileRows.Where(f => f.PossibleDuplicate == false), new ParallelOptions { MaxDegreeOfParallelism = maxParallels }, (fr) =>
        {
            foreach (FileRow cr in copiedRows)
            {
                // don't compare a row against its copy, or if it's already been processed
                if (cr.OrderBy != fr.OrderBy && cr.PossibleDuplicate == false)
                {
                    // check criteria individially to save speed. Titles are fastest
                    if (TitleMatch(fr.GetFieldByOrdinal((int)fc.TitleOrdinal), cr.GetFieldByOrdinal((int)fc.TitleOrdinal)))
                    {
                        // if true, it's worth checking surnames
                        if (SurnameMatch(fr.GetFieldByOrdinal(fc.SurnameOrdinal), cr.GetFieldByOrdinal(fc.SurnameOrdinal)))
                        {
                            // forenames have most lax criteria, so check last
                            if (ForenameMatch(fr.GetFieldByOrdinal(fc.ForenameOrdinal), cr.GetFieldByOrdinal(fc.ForenameOrdinal))
                                    && fr.CleanedAddress == cr.CleanedAddress )
                            {
                                fr.PossibleDuplicate = true;
                                break;
                            }
                        }
                    }
                }
            }
        });
    }
}

Answer 1

Encapsulation

Does a user of your class need to know about the methods in question (the one you had public) if no you would break the encapsulation of the class by exposing details to a user which he/she doesn't need to know.

If you decide to use only one public method you would need to do a null check at the very first start of the DedupeFile() method, otherwise you would expose the inner workings of the BuildNameKeys() method because this method would throw a NullReferenceException by the call to ModelHelper.GetOrdinalByAssignedLabel(fc.Mappings, "DOB");

SRP

If the responsibilty of the class is the deduplication of data, then any helper/validation method should be private to that class. Except they will be used in another class as well. In this case you should extract these to a separate class.

I tend to say that all of this matching methods should be in a separate class. Thats because the whole matching isn't really a responsibility of the DeduplicateIndividuals class. The responsibility is to check if there are (possible) duplicates. Having a class FilecontentMatcher which is used by the DeduplicateIndividuals class would make more sense IMO. Separating these concerns will help to satisfy the open closed principle too.

Comments

Some of your comments are bad like e.g // if true, it's worth checking surnames which doesn't tell anything interesting about the code. Some are missing like e.g for the ForenameMatch() method. There you should explain why

   if(forename1.FirstCharacter() == forename2.FirstCharacter())
    {
        return true;
    }  

is sufficient to return true

---

In the FuzzyMatching() method this: cr.PossibleDuplicate == false Will always evaluate to true because

List<FileRow> copiedRows = new List<FileRow>(fc.FileRows.Where(f => f.PossibleDuplicate == false));  

which makes this condition superflous.

---

Magic numbers

You have some magic numbers in your code which should be extracted to meaningful constants. Like e.g

private static bool SurnameMatch(string surname1, string surname2)
{
    return JaroWinkler.Proximity(surname1, surname2) >= 0.75;
}  

It would be much clearer to the reader of this code if it would say e.g

private static bool SurnameMatch(string surname1, string surname2)
{
    return JaroWinkler.Proximity(surname1, surname2) >= minimumSurenameProximity;
}

---

Naming

MrAndMrs is really a strange name for a method. You should try to find a better name which describes what the method is doing. If you or Sam the maintainer will read this method name someday somewhere in the code you or Sam will need to visit that method to get its meaning.

You should always use names as descriptive as possible if you name things.

Answer 2

Since you're interested in testability, my main issue with your code is the use of JaroWinkler and ModelHelper directly in your code. I would expect these "external" classes to be injected so you could stub them, otherwise how would you know in your unit testing whether the result you're getting is from your code or the external code?

But then they're all static classes which makes things awkward. I'd consider wrapping them in adapter classes which could be stubbed. Your own code being static doesn't help either.