3
\$\begingroup\$

The struct below parses full name into first name, middle name, last name and suffix.

Code

/// <summary>
///     Represents a person's name.
/// </summary>
public struct FullName : IEquatable<FullName>, IFormattable
{
    /// <summary>
    ///     Initializes a new instance of the <see cref="FullName" /> struct.
    /// </summary>
    /// <param name="firstName">The given name.</param>
    /// <param name="lastName">The family name.</param>
    /// <param name="middleName">The middle name.</param>
    /// <param name="suffix">The suffix.</param>
    public FullName(string? firstName, string? lastName = default, string? middleName = default,
        string? suffix = default)
    {
        FirstName = firstName;
        MiddleName = middleName;
        LastName = lastName;
        Suffix = suffix;
    }

    /// <summary>
    ///     Gets or sets the person's first name.
    /// </summary>
    public string? FirstName { get; set; }

    /// <summary>
    ///     Gets or sets the middle name.
    /// </summary>
    public string? MiddleName { get; set; }

    /// <summary>
    ///     Gets or sets the person's last name.
    /// </summary>
    public string? LastName { get; set; }

    /// <summary>
    ///     Gets or sets the suffix. (e.g. PhD, Jr, etc)
    /// </summary>
    public string? Suffix { get; set; }

    /// <summary>
    ///     Parses the specified text.
    /// </summary>
    /// <param name="text">The text.</param>
    public static FullName Parse(string text)
    {
        if (string.IsNullOrEmpty(text))
        {
            return new FullName();
        }

        var parts = text.Split(' ');

        string? Get(int idx)
        {
            return idx >= parts.Length ? null : parts[idx];
        }

        switch (parts.Length)
        {
            case 1:
            case 2:
                return new FullName(Get(0), Get(1));

            case 3:
                var last = Get(2);

                return IsSuffix(last)
                    ? new FullName(Get(0), Get(1), default, last)
                    : new FullName(Get(0), Get(2), Get(1));

            case 4:
                return new FullName(Get(0), Get(2), Get(1), Get(3));
        }

        return new FullName();
    }

    #region IFormattable

    /// <summary>Converts to string.</summary>
    /// <returns>A <see cref="System.String" /> that represents this instance.</returns>
    public override string ToString()
    {
        return string.Join(" ", FirstName, MiddleName, LastName, Suffix).Replace("  ", " ").Trim();
    }

    /// <summary>
    ///     Converts to string.
    /// </summary>
    /// <param name="format">The format.</param>
    /// <returns>
    ///     A <see cref="System.String" /> that represents this instance.
    /// </returns>
    public string ToString(string format)
    {
        return ToString(string.Concat("{0:", string.IsNullOrEmpty(format) ? "G" : format, '}'), Formatter);
    }

    /// <summary>
    ///     Converts to string.
    /// </summary>
    /// <param name="format">The format.</param>
    /// <param name="formatProvider">The format provider.</param>
    /// <returns>
    ///     A <see cref="System.String" /> that represents this instance.
    /// </returns>
    public string ToString(string? format, IFormatProvider? formatProvider)
    {
        return string.Format(formatProvider ?? Formatter, string.IsNullOrEmpty(format) ? "{0:G}" : format, this);
    }

    public static implicit operator string(FullName obj)
    {
        return obj.ToString();
    }

    public static explicit operator FullName(string obj)
    {
        return Parse(obj);
    }

    #endregion IFormattable

    #region IEquatable

    /// <summary>
    ///     Determines if the specified names are equal.
    /// </summary>
    /// <param name="a">The first name.</param>
    /// <param name="b">The other name.</param>
    /// <param name="comparison">The comparison (defaults to <see cref="StringComparison.InvariantCultureIgnoreCase" />).</param>
    public static bool Equals(FullName a, FullName b,
        StringComparison comparison = StringComparison.InvariantCultureIgnoreCase)
    {
        return
            string.Equals(a.FirstName, b.FirstName, comparison) && string.Equals(a.MiddleName, b.MiddleName, comparison)
                                                                && string.Equals(a.LastName, b.LastName, comparison)
                                                                && string.Equals(a.Suffix, b.Suffix, comparison);
    }

    public static bool operator ==(FullName x, FullName y)
    {
        return Equals(x, y);
    }

    public static bool operator !=(FullName x, FullName y)
    {
        return !Equals(x, y);
    }

    /// <summary>
    ///     Indicates whether the current object is equal to another object of the same type.
    /// </summary>
    /// <param name="other">An object to compare with this object.</param>
    /// <returns>
    ///     true if the current object is equal to the <paramref name="other">other</paramref> parameter; otherwise, false.
    /// </returns>
    public bool Equals(FullName other)
    {
        return Equals(this, other);
    }

    /// <summary>
    ///     Determines whether the specified <see cref="System.Object" />, is equal to this instance.
    /// </summary>
    /// <param name="obj">The <see cref="System.Object" /> to compare with this instance.</param>
    /// <returns>
    ///     <c>true</c> if the specified <see cref="System.Object" /> is equal to this instance; otherwise, <c>false</c>.
    /// </returns>
    public override bool Equals(object? obj)
    {
        return obj is FullName name && Equals(this, name);
    }

    public override int GetHashCode()
    {
        return
            FirstName?.GetHashCode() ?? 0 ^
            MiddleName?.GetHashCode() ?? 0 ^
            LastName?.GetHashCode() ?? 0 ^
            Suffix?.GetHashCode() ?? 0;
    }

    #endregion IEquatable

    #region Backing Members

    internal static bool IsSuffix(string text)
    {
        // NOTE: A suffix is a string that ends with a period (.)
        // or has a uppercase beyond the first character.

        if (string.IsNullOrEmpty(text))
        {
            return false;
        }

        if (text[^1] == '.')
        {
            return true;
        }

        for (var i = 1; i < text.Length; i++)
        {
            var c = text[i];
            if (char.IsLetter(c) && char.IsUpper(c))
            {
                return true;
            }
        }

        return text.Length is 3 or 4 || IsAllCaps(text);
    }

    internal static bool IsAllCaps(string text)
    {
        return text.All(c => !char.IsLetter(c) || !char.IsLower(c));
    }

    private static NameFormatter? _formatter;

    private static NameFormatter Formatter => _formatter ??= new NameFormatter();

    #endregion Backing Members
}

internal class NameFormatter : ICustomFormatter, IFormatProvider
{
    public string Format(string? format, object? arg, IFormatProvider? formatProvider)
    {
        if (arg is FullName name)
        {
            var n = format.Length;
            var builder = new StringBuilder();

            for (var i = 0; i < n; i++)
            {
                char c;
                switch (c = format[i])
                {
                    default:
                        builder.Append(c);
                        break;

                    case 'G':
                        builder.Append(name.ToString());
                        break;

                    case 'S':
                        builder.Append(string.Join(" ", name.FirstName, name.LastName).Trim());
                        break;

                    case '1':
                    case 'f':
                    case 'F':
                        builder.Append(name.FirstName);
                        break;

                    case '2':
                    case 'm':
                    case 'M':
                        builder.Append(name.MiddleName);
                        break;

                    case '3':
                    case 'l':
                    case 'L':
                        builder.Append(name.LastName);
                        break;

                    case '\\': /* Escape */
                        builder.Append(i + 1 < n ? format[++i] : c);
                        break;
                }
            }

            return builder.ToString().Trim();
        }

        return GetFallbackFormat(format, arg);
    }

    public object? GetFormat(Type? formatType)
    {
        return formatType == typeof(ICustomFormatter) ? this : null;
    }

    private string? GetFallbackFormat(string format, object? arg)
    {
        if (arg is IFormattable formattable)
        {
            return formattable.ToString(format, CultureInfo.CurrentCulture);
        }

        return arg != null ? arg.ToString() : string.Empty;
    }
}

Unit tests

public sealed class FullNameTests
{
    [Theory]
    [InlineData("Иван Иванов", "Иван", default, "Иванов")]
    [InlineData("Анна-Мария Драганова Иванова", "Анна-Мария", "Драганова", "Иванова")]
    public void Build_ShouldReturnCorrectUsernamePassword_WhenGivenFullName(
        string fullName,
        string expectedFirstName,
        string expectedMiddleName,
        string expectedLastName)
    {
        // Arrange


        // Act
        var actual = FullName.Parse(fullName);

        // Assert
        actual.FirstName.Should().Be(expectedFirstName);
        actual.MiddleName.Should().Be(expectedMiddleName);
        actual.LastName.Should().Be(expectedLastName);
    }
}

\$\endgroup\$

5 Answers 5

5
\$\begingroup\$

Lots of questions here.

/// <param name="firstName">The given name.</param>
/// <param name="lastName">The family name.</param>
/// <param name="middleName">The middle name.</param>
/// <param name="suffix">The suffix.</param>

Why are you expecting first-last-middle? Why would someone not put their middle name in the middle?


public string? FirstName { get; set; }

For the name properties, why do they have public setters? Are you expecting external classes to change someone's name? I would make them private or protected: public string? FirstName { get; protected set; }, etc.


for (var i = 1; i < text.Length; i++)
{
    var c = text[i];
    if (char.IsLetter(c) && char.IsUpper(c))
    {
        return true;
    }
}

For checking if a name is a suffix or not, why do you keep looking past the second letter? If someone's name were Betsy DeVos (American politician) then the last name DeVos would trigger as a suffix because you keep looking for a capital letter. I would flip the check here, if any letter is NOT capital then it's false (not a suffix). If you check all the letters and don't return false; then you should return true.


if (string.IsNullOrEmpty(text))
{
    return new FullName();
}

Why would you allow an empty name? You're in Parse(), not TryParse(), so (IMO) it's okay to throw here. I'd do an invalid argument exception. Speaking of which, I think you could easily implement a TryParse as well, if you throw from Parse:

public static bool TryParse(string text, out FullName name)
{
    name = new FullName();
    try
    {
        name = Parse(text);
        return true;
    }
    catch
    {
        return false;
    }
}

anything that's not valid causes Parse to fail, which would cause TryParse to return false. Since it's returning false the out name is assumed to be invalid.


return text.Length is 3 or 4 || IsAllCaps(text);

Back on the IsSuffix check, why does it matter about the length? I think your code would take Lana Del Rey (American singer) and treat her last name as a suffix because it's three letters long.


switch (parts.Length)
{
    // ...
    case 4:
        return new FullName(Get(0), Get(2), Get(1), Get(3));
}

Last comment here, George H. W. Bush (former American president) has two middle names, which would trigger Bush to get treated as a suffix, both for being 4 letters long and for being the fourth argument in the name. And, because of the first-last-middle scheme, his name would get scrambled. This is also not mentioning people like José María Álvarez del Manzano y López del Hierro (Spanish politician), whose paternal surname is Álvarez del Manzano.


Ultimately, if you've got the ability to enforce first-last-middle, then I'd argue to enforce a comma to separate the suffix, like Charles Philip Arthur George, III or Saul Goodman, Esq.

\$\endgroup\$
8
  • \$\begingroup\$ Re-reading my answer here, and I disagree with my own statement, I would flip the check here, if any letter is NOT capital then it's false (not a suffix). You could have something like I mention, Saul Goodman, Esq which would fail to register Esq (or Jr, Sr, etc.) as a suffix because they have lower-case letters. Of course, Jr or Sr fail as suffixes on all the other checks, too, because they're not 3 or 4 characters long, aren't all caps, and don't have a capital letter at or after the second letter. Really you'll have to enforce some other signaling mechanism (like a comma). \$\endgroup\$
    – Chuck
    Commented Oct 12, 2022 at 20:01
  • 1
    \$\begingroup\$ Thank you very much! I adjusted the code based on your review and there are so complex names, so I guess I would focus on these around me. \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 14:45
  • 1
    \$\begingroup\$ @nop Happy to help. Names are a really, really difficult problem and I think you're liable to discover tons of edge cases. Unfortunately I think the only way to really get around this would be to prompt users for their name parts individually and then build from those parts. To that end, I'd probably have wrapper classes like FirstName, MiddleName, etc., where each wrapper class only holds a string. Prompt the user for name parts, and then you can build with Public FullName(FirstName? firstName, List<MiddleName> middleNames, LastName? lastName, Suffix? suffix). \$\endgroup\$
    – Chuck
    Commented Oct 13, 2022 at 14:51
  • 1
    \$\begingroup\$ Wrapping the strings in the thin classes like FirstName, etc. lets your constructor pick apart which name is which without you having to do all this guesswork in your code. Again though, this requires a series of prompts beyond the scope of the constructor to create the name parts. \$\endgroup\$
    – Chuck
    Commented Oct 13, 2022 at 14:52
  • 1
    \$\begingroup\$ I tried to do that but I'm not sure how it is supposed to work yet pastebin.com/jQSh3XVA. It's basically what Jesse suggested. \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 15:00
3
\$\begingroup\$

The answer from Chuck covers a lot of good ground. I'd take your code a step further and make your first name, last name, middle name, and suffix their own types (structs) as well. That way, your FullName code (or calling code) won't accidentally mix them up. For instance, here's a now-type-safe FirstName:

public struct FirstName : IEquatable<FirstName>
{
    private readonly string? _value;

    public FirstName(string? value) => this._value = value;

    public static explicit operator string?(FirstName firstName) => firstName.ToString();

    public static implicit operator FirstName(string? value) => new FirstName(value);

    public static bool operator ==(FirstName x, FirstName y) => Equals(x, y);

    public static bool operator !=(FirstName x, FirstName y) => !Equals(x, y);

    public static bool Equals(FirstName a, FirstName b, StringComparison comparison = StringComparison.InvariantCultureIgnoreCase) => string.Equals(a.ToString(), b.ToString(), comparison);

    public bool Equals(FirstName other) => Equals(this, other);

    public override bool Equals(object? obj) => obj is FirstName firstName && Equals(this, firstName);

    public override int GetHashCode() => this._value?.GetHashCode() ?? 0;

    public override string? ToString() => this._value ?? string.Empty;
}
\$\endgroup\$
3
  • \$\begingroup\$ Thank you for your answer! I like it, however, I'm not sure exactly how to achieve that in the FullName struct. May you have a look? pastebin.com/jQSh3XVA \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 14:57
  • 1
    \$\begingroup\$ @nop here you go: pastebin.com/bfpiWZ4U and pastebin.com/rGgXShte \$\endgroup\$ Commented Oct 13, 2022 at 15:50
  • \$\begingroup\$ Thank you very much! :) \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 16:00
2
\$\begingroup\$

Others have already reviewed your code so, let me focus only on your unit test

What I've liked

  • You have used PUT (parameterized unit test) instead of copy-pasting
  • You have used Arrange-Act-Assert comments

What could be improved

These are separate test cases

  • Your first test case performs a full name parsing without middle name,
    • whereas the second test case performs a full name parsing with middle name
  • Even though they seem similar they are different test cases
    • The main aim of writing unit tests is to help you find (locate) bugs in your code
    • If you have the same test for both test cases then this does not really help you to locate the problem

Missing test cases

  • You did not cover all positive test cases (for example how to handle suffix)
    • You haven't shared with us any test cases regarding ToString w/o formatting
    • Equality related tests are also missing
  • You haven't shared with us any negative test cases (for example where you receive a new FullName without any parsed substrings)

Test case naming

  • I would suggest to use the Given-When-Then structure, like

    • GivenANameWithFirstAndLastNames_WhenIParseItAsFullName_ThenItReturnsAFullNameWhereOnlyTheFirstAndLastNamesArePopulated
    • GivenANameWithoutSuffix_WhenIParseItAsFullName_ThenItReturnsAFullNameWhereAllNamesArePopulatedExceptSuffix
    • etc.

Single assertion

var expectedFullName = new
{
  FirstName = expectedFirstName,
  LastName = expectedLastName 
}.ToExpectedObject();

expectedFullName.ShouldMatch(actualFullName);
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Thank you very much! :) I always wondered when I have to put these scenarios into different tests and that just found a bug for me! Will reply back with the corrected tests :) \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 15:14
  • \$\begingroup\$ pastebin.com/2tfBB2nG - here is the updated FullName struct and its tests. I kept the current convention as that's what Nick Chapsas used in his latest workshop github.com/Hulkstance/testing-workshop-ndcsydney-2022. And thanks again, you showed me how to do tests properly. :) Hopefully they are okay. \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 15:36
  • 1
    \$\begingroup\$ @nop They seem fine, except those where you compare two full names. Do not assert on the result of == operator. Use the proper equality assertion for that. \$\endgroup\$ Commented Oct 14, 2022 at 18:03
  • 1
    \$\begingroup\$ @nop Also I still some test cases. Run a code coverage gathering tool to see which lines are not covered unit tests. \$\endgroup\$ Commented Oct 14, 2022 at 18:06
  • 1
    \$\begingroup\$ i.imgur.com/0WZVihK.png pastebin.com/iMU09WmK it says equality and not equality were untested, as well as GetHashCode. \$\endgroup\$
    – nop
    Commented Oct 14, 2022 at 20:50
2
\$\begingroup\$

Weak Hash

Your hash function is weak, and will generate more collisions than necessary: first and middle names can be used interchangeably, and some of them can also be used as last names, and I guess some suffixes like "Junior" could also be used as a first name.

However, by Xoring the hashes of the various fields with no additional processing, you don't make any distinction about the fields. As such, you will get frequent hash collisions for all cases with inversions in the various name parts: "John Henri Doe" and "Henri John Doe", "Carter Madison" and "Madison Carter", "John Doe Junior" and "Junior John Doe", etc.

You could resolve this by shifting bits of the hash codes of the different fields before xoring or use HashCode.Combine:

public override int GetHashCode()
{
    return HashCode.Combine(
        FirstName?.GetHashCode() ?? 0,
        MiddleName?.GetHashCode() ?? 0,
        LastName?.GetHashCode() ?? 0,
        Suffix?.GetHashCode() ?? 0);
}

Names are hard

The assumption that the names are going to fit the <first> <middle> <last> <suffix> format is very restrictive. If you use this kind of structure for any real-world use, it will most likely fail quite often.

People can have multiple first, middle or last names, or might want to use both their maiden name and spouse's name. These cases are quite common even for people with English American names, and they will all break your parsing logic. But people from all over the world with various countries will have names based on different conventions with different logic. And people emigrate, so you can't assume that you aren't going to deal with these cases based on your target demographic.

I can see you tried to be accommodating, by allowing any field to be null, but it's not enough, my real name won't fit in that struct, and if I can thing of ways to coerce it into that format, it won't be parsed correctly (I have 3 first names and a last name, which is pretty common in France).

The truth is, there is no good solution to this problem. You can either have a single string for a full name, allowing any user to use any format they require for their full name, or you can try to elaborate on your class to account for more cases (or fewer, limiting the fields to first and last names).

Both options have issues, the former makes it hard to normalize names and you get unreliable precision as different people have different understandings of what is a "full name", the latter makes it impossible to account for all cases.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Please note that HashCode struct is only available since .Net Core 2.1. \$\endgroup\$ Commented Oct 13, 2022 at 11:00
  • \$\begingroup\$ Thank you for the answer! \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 14:26
2
\$\begingroup\$

Good Things

Your code is nicely formatted, quite easy to read, and decently organized.

What is Missing

I do not see a Nickname or PreferredName property. Consider someone named "William Henry O'Toole" but he likes to be called "Hank" (a short form of Henry, his middle name).

The difficulty with a PreferredName is that you many not want to use it for identity purposes. This throws a complication in how to compare records, when certain properties on the records do not need to be used when determining equality.

Names are Hard

My biggest concern is that you provided too simple a model for something that is very complex.

Helpful links:

Falsehoods about Names

Falsehoods Programmers Believe about Names

For instance, your code allows for someone not to have a middle name. Great. What about people who have 2 middle names?

Your parser splits full names based on a blank. I knew a woman whose first name was "Bobbi Jo". She would emphatically state that her middle name is NOT "Jo". Nor was her first name "Bobbi".

What about Dutch family names like "van der Winkle"?

Consider the famous Spanish painter Picasso. Or we might call him Pablo Picasso. Yet his full name at birth was: Pablo Diego José Francisco de Paula Juan Nepomuceno María de los Remedios Cipriano de la Santísima Trinidad Ruiz y Picasso.

I've seen (either here or in your related posts), where "PhD" is a suffix. Things like "M.D." and "PhD" are not a part of the name. They are extraneous titles. Sort of like Honorifics (Mr., Mrs., Lord, Judge, The Honorable, etc.) preceding a name, but they trail the name. For example, in the U.S. attorneys may end their name with "Esquire" or "esq.". What if Perry Mason (fictional TV attorney in 1950's) was also a Junior, with a medical degree and another PhD (besides the Juris Doctorate): his name might be "Perry Mason, Jr., M.D., PhD, esq.". Or it might be "Perry Mason, Jr., PhD, M.D., Esquire". Both would refer to the same person.

I would suggest having your code focus on the key parts of a name used in a person's identity. That would be to skip over the titles and pedigrees, but include the "Jr.", "Sr.", "III", etc. In the example from the previous paragraph, we know we have a person identified as "Perry Mason, Jr.".

And by all means review the referenced links at your leisure. The first one has a lot of interesting topics besides human naming.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you for the information! I didn't think about those naming details. \$\endgroup\$
    – nop
    Commented Oct 13, 2022 at 14:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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