Testing PersonFirstName class as a Value Object

Question

For reference:

A Value Object is an immutable type that is distinguishable only by the state of its properties. That is, unlike an Entity, which has a unique identifier and remains distinct even if its properties are otherwise identical, two Value Objects with the exact same properties can be considered equal.

Please see the code below:

public class PersonFirstName
{
    //public string FirstName { get; set; }

    private string _firstName;

    public PersonFirstName(string firstName)
    {
        if (firstName == null)
            throw new ArgumentNullException("Surname");
        if (!PersonFirstName.IsValid(firstName))
            throw new ArgumentException("Invalid value.", "Surname");
        this._firstName = firstName;
        FirstName = firstName;
    }

    public PersonFirstName() //for NHibernate
    {
    }

    public static bool IsValid(string candidate)
    {
        if (string.IsNullOrWhiteSpace(candidate))
            return false;
        if (candidate.Length > 255)
            return false;
        return true;
    }

    public string FirstName
    {
        get { return _firstName; }
        private set 
        {
            _firstName = value; 
        }
    }

    public override bool Equals(object obj)
    {
        PersonFirstName PersonFirstName = obj as PersonFirstName;
        if (ReferenceEquals(PersonFirstName, null))
            return false;
        return _firstName == PersonFirstName.FirstName;
    }

    public override int GetHashCode()
    {
        return _firstName.GetHashCode();
    }
}

and the Unit Test below:

[Test]
public void Domain_FirstNameValue_ValueObjectCreated()
{
    //Arrange
    var PersonFirstName1 = new PersonFirstName("Ian");
    var PersonFirstName2 = new PersonFirstName("Ian");
    //Act
    var firstNameCompareResult = PersonFirstName1.Equals(PersonFirstName2);
    //Assert
    Assert.True(firstNameCompareResult);
}

I would be grateful for comments on the quality of this test.

Answer 1

A few notes:

• The argument exceptions contain an incorrect parameter name ("Surname"). Use nameof(firstName) instead: this allows the compiler to verify that you're referencing an existing identifier, and the refactoring tools of your IDE can automatically update it when you rename that parameter.
• Why are you setting both this._firstName and FirstName in the constructor? That's doing double work.
• A read-only automatic property seems appropriate here: public string FirstName { get; }. This can only be assigned within a constructor, similar to readonly fields, and doesn't require you to explicitly create a backing field.
• I see a disabled property with a public setter. If it wasn't disabled, then FirstName instances could not guarantee that their GetHashCode would always return the same value. That could lead to subtle bugs such as failing dictionary lookups, so you did well to make your class immutable.
• You may want to implement IEquatable<T>. It's not a replacement for Equals and GetHashCode, but it lets you provide a strongly-typed equality check, which can prevent unnecessary type-checks or boxing/unboxing, and it clearly signals intent. Likewise, if you need to order or sort these first-names, take a look at IComparable and IComparable<T>.
• Why does IsValid not allow names longer than 255 characters? If there's a good reason for that, then why isn't it documented? And does that restriction always apply, or only in a certain context? And is a simple false or ArgumentException sufficient for the caller, or do they need to distinguish between different validation failures?
• The test method is incorrectly named: according to the name, it's testing object creation, but it's actually testing equality. Domain_EqualFirstNameValues_Equals would be more accurate.
• Assert.True has an overload that accepts a failure message. A good message (perhaps including the values that are involved) can make it easier to determine why a test is failing.

Answer 2

I would also add a "sanity check" test which verifies that the two names are different, false is returned:

public void Domain_FirstNameValue_ValueObjectCreated()
{
    //Arrange
    var PersonFirstName1 = new PersonFirstName(Guid.NewGuid().ToString());
    var PersonFirstName2 = new PersonFirstName(Guid.NewGuid().ToString());
    //Act
    var firstNameCompareResult = PersonFirstName1.Equals(PersonFirstName2);
    //Assert
    Assert.False(firstNameCompareResult);
}

Otherwise your test can pass with a degenerate implementation of Equals:

public override bool Equals(object obj)
{
    return true;
}

Granted in this case you'd probably spot it but the principle is to make sure your test result isn't a false positive :)

I'd also try to remove the hardcoded values from the test, just to ensure you don't run into the same issues.

e.g.

public void Domain_FirstNameValue_ValueObjectCreated()
{
    //Arrange
    var name = Guid.NewGuid().ToString();
    var PersonFirstName1 = new PersonFirstName(name);
    var PersonFirstName2 = new PersonFirstName(name);
    //Act
    var firstNameCompareResult = PersonFirstName1.Equals(PersonFirstName2);
    //Assert
    Assert.True(firstNameCompareResult);
}

With both this and the sanity check test, it's much less likely that any degenerate implementation can pass both.

Also in the implementation itself I'd change if (ReferenceEquals) to just a plain null check - this is to indicate that the object passed is not an instance of PersonFirstName as opposed to if it's a null reference in general.

e.g.

public override bool Equals(object obj)
{
    var PersonFirstName = obj as PersonFirstName;
    if (PersonFirstName == null) return false;
    return _firstName == PersonFirstName.FirstName;
}

Answer 3

The restriction on 255 characters is probably too weak. In MySQL for example, a VARCHAR(255) field is 255 bytes long, not 255 characters. If you ever want to save non-English names, the limit may be as small as 63 characters since the UTF-8 encoding for emojis (among others) needs 4 bytes per character.

Local variable names usually start with a lowercase letter.