1
\$\begingroup\$

I asked this question yesterday: Entity and Value Object are not part of the Ubiqtious language. Should this stop me from using them?.

I originally decided to introduce a ValueObject class as described here: Generic Value Object Equality, which contained equality methods for all value objects i.e. all Value Objects inherited from ValueObject the value object class. However, this approach was criticised.

Please see the code below:

public sealed class Surname : IEquatable<Surname>
{
    private readonly string _value;
    public Surname(string surname)
    {
        if (surname.Length==0)
            throw new ArgumentException("Invalid value.", "Surname");
        this._value = surname;
    }

    public string Value
    {
        get { return _value; }
    }

    public static bool operator ==(Surname surname1, Surname surname2)
    {
        if (!ReferenceEquals(surname1, null) &&
            ReferenceEquals(surname2, null))
        {
            return false;
        }
        if (ReferenceEquals(surname1, null) &&
             !ReferenceEquals(surname2, null))
        {
            return false;
        }
        return surname1.Equals(surname2);
    }

    public static bool operator !=(Surname surname1, Surname surname2)
    {
        return !(surname1== surname2);
    }

    public bool Equals(Surname other)
    {
        if (other != null)
        {
            return _value == other._value;
        }
        return base.Equals(other);
    }

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

    public override int GetHashCode()
    {
        unchecked
        {
            int hash = 17;
            hash = hash * 23 + _value.GetHashCode();
            return hash;
        }
    }
}

It is a DDD Value Object for a Surname.

I have added some methods for equality and:

  1. I would like some feedback on the quality of them.

  2. This question (point one) links to a presentation by Eric Evans where he appears to advise against introducing equality for Entities. Is it advisable to always compare entities by reference these days?

\$\endgroup\$
  • 4
    \$\begingroup\$ It's good that you also overrode GetHashCode(); many people neglect to do this when overriding Equals(). With that said, adding 17*23 is a linear transformation of the underlying hashcode and doesn't add any extra complexity or have any benefit. Just return _value.GetHashCode() \$\endgroup\$ – Zack Jan 30 '18 at 18:42
  • \$\begingroup\$ I got the hashcode implementation from here: stackoverflow.com/questions/263400/… (see Jon Skeets answer). Do you have any other thoughts? What about having the unchecked bit. \$\endgroup\$ – w0051977 Jan 30 '18 at 18:45
  • 3
    \$\begingroup\$ That post applies if there are multiple fields/properties. For example, if this were a Person class, with FirstName and LastName properties, you could do (17 * (23 * First.GetHashCode)) * 23 * Last.GetHashCode. Another good option is to use xor (^): First.GetHashCode ^ Last.GetHashCode. You only have one field, _value, so anything you do is just a linear transformation of the underlying hash code, and the chance of a collision is unchanged \$\endgroup\$ – Zack Jan 30 '18 at 18:51
  • \$\begingroup\$ @Zack,Could you take a look at my other question here?: codereview.stackexchange.com/questions/186590/… \$\endgroup\$ – w0051977 Feb 2 '18 at 13:55
5
\$\begingroup\$

I don't see any advantage that this class provides over a simple string. I expect a Surname to be at least case-insensitive but this class' equality is exactly the same as if it was a string.

I also don't understand why you change the hashcode of the string? You could just return the one that _value returns... but agian, this would be the same as an the one of an ordinary string.

Take a look a the StringComparer.OrdinalIgnoreCase because this is what you actually need... I guess. It would make sense.

As far as a Surname is concerned I also expect it to yield true when comparing "John" with " John ". So you might want to Trim the value.


This kind of case-insensitive trimmed string is such a common use case that I have special class for that. It saves a lots of headaches when working this kind of data.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I call it SoftString and you can take a look at it here if you like. (There are also four other files implementing some operators for it etc.) \$\endgroup\$ – t3chb0t Jan 30 '18 at 19:02
  • \$\begingroup\$ The reason I am using a surname object is to remove primitive obsession to make the domain model easier to talk about: lostechies.com/jimmybogard/2007/12/03/… i.e. talk about a surname rather than a string. What do you think? I may just use a string . \$\endgroup\$ – w0051977 Jan 30 '18 at 19:37
  • \$\begingroup\$ @w0051977 I know this ;-) and I support this idea but only if the custom type has some additional value like it encapsulates some logic that you otherwise would repeat in multiple places. E.g. case-insensitive comparison or trimming or other validations. Your type however doesn't do anything that a string already can't. \$\endgroup\$ – t3chb0t Jan 30 '18 at 19:41
  • \$\begingroup\$ Thanks. I see what you mean. I guess it would be pointless creating a classes for salary and date of birth as well? \$\endgroup\$ – w0051977 Jan 30 '18 at 19:43
  • \$\begingroup\$ @w0051977 if there is nothing special about them then I it's just an ordinary number and a date. But... if you created a DateOfBirth class that couldn't be initialized with a future date... well, then we can discuss this idea ;-] or an Age class that couldn't be negative. There needs to be something more that makes them more useful than a base type. The name does not count ;-] \$\endgroup\$ – t3chb0t Jan 30 '18 at 19:50

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.