2
\$\begingroup\$

Please see the code below:

public sealed class DateOfBirth : IEquatable<DateOfBirth>, IComparable<DateOfBirth>
    {
        private readonly DateTime _value;

        public DateTime Value
        {
            get { return _value; }
        }
        public DateOfBirth(DateTime dateOfBirth)
        {
            if(dateOfBirth == DateTime.MinValue)
                throw new ArgumentException("Invalid value.", "DateOfBirth");
            this._value= dateOfBirth;
        }

        private static int Comparison(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            if (ReferenceEquals(dateOfBirth1, dateOfBirth2))
                return 0;
            else if (ReferenceEquals(dateOfBirth1, null))
                return -1;
            else if (ReferenceEquals(dateOfBirth2, null))
                return 1;
            if (dateOfBirth1._value < dateOfBirth2._value)
                return -1;
            else if (dateOfBirth1._value == dateOfBirth2._value)
                return 0;
            else if (dateOfBirth1._value > dateOfBirth2._value)
                return 1;
            return 0;
        }

        public int CompareTo(DateOfBirth other)
        {
            if (other != null)
                return this._value.CompareTo(other._value);
            else
                throw new ArgumentNullException("DateOfBirth");
        }


        public static bool operator ==(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return Comparison(dateOfBirth1, dateOfBirth2) == 0;
        }

        public static bool operator !=(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return !(dateOfBirth1 == dateOfBirth2);
        }

        public static bool operator <(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return Comparison(dateOfBirth1, dateOfBirth2) < 0;
        }

        public static bool operator >(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return Comparison(dateOfBirth1, dateOfBirth2) > 0;
        }

        public static bool operator <=(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return Comparison(dateOfBirth1, dateOfBirth2) <= 0;
        }

        public static bool operator >=(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return Comparison(dateOfBirth1, dateOfBirth2) >= 0;
        }

        public bool Equals(DateOfBirth other)
        {
            if (ReferenceEquals(other, null))
                return false;
            return _value == other._value;
        }

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

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

I realise it could be argued as over engineering a simple date of birth field. I am more interested whether or not the nine comparisons (https://ericlippert.com/2013/10/07/math-from-scratch-part-six-comparisons/) are implemented correctly. This article helped me: https://blogs.msdn.microsoft.com/abhinaba/2005/10/11/c-comparison-operator-overloading-and-spaceship-operator/ - the only concern I have about the code in this article is that the Comparison method does not do this (which my code does):

if (ReferenceEquals(dateOfBirth1, dateOfBirth2))
                return 0;
            else if (ReferenceEquals(dateOfBirth1, null))

Q1) Is the code above fit for purpose (are there any bugs)?

Q2) Is a DateTime data type (DateOfBirth._value) suitable for a Date of Birth? return -1;

Q3) Is the validation in the constructor fit for purpose?

\$\endgroup\$
  • \$\begingroup\$ My .Net knowledge is a little dated (no pun intended), but it seems like it would be easier to have class DateOfBirth extends DateTime {} and leave it at that. You are more or less creating an exact copy of DateTime under a different name. \$\endgroup\$ – Zack Feb 2 '18 at 15:44
  • \$\begingroup\$ DateTime is a struct, which does not support inheritance \$\endgroup\$ – JamesFaix Feb 2 '18 at 16:32
  • \$\begingroup\$ I figured there would be something. One could still map all the functions to call the underlying DateTime methods instead. E.g. bool Equals(object obj) { return _value.Equals(obj); }, bool operator <=(DateOfBirth dob1, DateOfBirth dob2) { return dob1.Value <= dob2.Value; }, etc. You could even declare DateOfBirth as a struct instead of a class, and/or give it an implicit cast to DateTime so that for all the functions expecting a DateTime, you could pass DateOfBirth directly without having to call .Value \$\endgroup\$ – Zack Feb 2 '18 at 17:24
  • \$\begingroup\$ I don't see where _value is assigned. \$\endgroup\$ – paparazzo Feb 3 '18 at 9:34
  • 1
    \$\begingroup\$ @Paparazzi in public DateOfBirth(DateTime dateOfBirth): this._value= dateOfBirth; \$\endgroup\$ – VisualMelon Feb 3 '18 at 10:16
3
\$\begingroup\$
  • 'fit for purpose' depends on what exactly that purpose is. Do you want to register the time of birth (as the Value property suggests), or only the date (as the class name seems to imply)? Does it need to handle birth dates before 0 A.D.? Does it need to reject non-existing dates (depending on actual historical data)?
  • If this class is meant to store a date only (not a time component), then it makes more sense to give it Year, Month and Day properties, rather than a Value property that exposes a time component.
  • DateTime.MinValue is the default value of DateTime, but why does that make it an invalid birth-date?
  • It may be useful to provide (explicit or implicit) conversions operators from and to DateTime, and perhaps parsing methods, depending on where and how this class is meant to be used. However...
  • ...the purpose of this class seems very narrow. Judging from your other posts, you are thinking about creating many such 'value' types? That's a lot of work (at least in C#), but what problem does it solve? Where's the added value? Are you worried that someone may accidentally assign an expiry-date to a birth-date field? Remember, all that extra code needs to be maintained, and there's a cost associated with that.

My impression is that yes, you are over-engineering this. I suspect that you'll find more value in writing automated tests (which, when done properly, should have caught that >= operator bug, and should catch expiry-date/birth-date mixups just as well) than in adding this sort of complexity. C# requires too much boiler-plate to make this approach viable, so unless lives are at stake I wouldn't bother.


EDIT: A few more notes:

  • In C# 7, ReferenceEquals(a, null) can be written more succinctly as a is null.
  • The null check in CompareTo uses the overloaded != operator, but you might as well use a reference check directly.
  • Why are the Equals methods not using Comparison? The same goes for the != operator: why doesn't it do Comparison(dateOfBirth1, dateOfBirth2) != 0?
\$\endgroup\$
  • \$\begingroup\$ The purpose is to remove primitive obsession and make the domain model easier to talk about with a Business Analyst. Plus the validation is in one place. Do you still think it is an overkill? \$\endgroup\$ – w0051977 Feb 2 '18 at 15:33
  • \$\begingroup\$ You're simply replacing primitive obsession with value type obsession. Why would public DateOfBirth DateOfBirth { get; } make talking with a business analyst easier than public DateTime DateOfBirth { get; }? If they say: 'Why do birth-dates contain a time? They shouldn't', then replace DateTime with a general-purpose Date type. But I see no point in duplicating DateTime. \$\endgroup\$ – Pieter Witvoet Feb 2 '18 at 15:47
  • \$\begingroup\$ If I used three properties for the date then what would I expose via getters i.e. three integers or a datetime. \$\endgroup\$ – w0051977 Feb 2 '18 at 16:39
  • \$\begingroup\$ Just think about what a date is: it's a year, month and day, right? So I'd expect a Date class to have a property for each of those. A date does not contain a time, so a DateTime property would only be confusing. \$\endgroup\$ – Pieter Witvoet Feb 2 '18 at 21:23
  • 1
    \$\begingroup\$ @t3chb0t there is always NodaTime. Having spent time with F#, I do rather wish I could one-line these sorts of types (i.e. which just wrap a name over another type, providing a decompose method (Value) and precluding cross-assignment); however, given the overhead, I agree with the answers that it is rarely worth it. \$\endgroup\$ – VisualMelon Feb 3 '18 at 10:25
2
\$\begingroup\$

ctor

It looks strange that the passed DateTime parameter is named dateOfBirth. A reader of the code could get confused because mostly you use DateOfBirth dateOfBirthX wher X stands for either 1 or 2.

CompareTo()

The code doesn't comply with the documentation of IComparable<T>.CompareTo which states in the remarks

By definition, any object compares greater than null, and two null references compare equal to each other.

meaning if other == null the method should return a value > 0. Throwing an exception would be quite confusing if e.g you have a List<DateOfBirth>, where it is legal to add null, and try to Sort() to get an ArgumentNullException.

Comparision()

This method should be simplified because you don't need to check if dateOfBirth1._value == dateOfBirth2._value if you at the end of the method return 0 anyway.

While we are at this if..else if..else if.... you should do yourself a favour and use braces {} although they might be optional. Omitting braces can lead to hidden and therfor hard to find bugs.

private static int Comparison(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
{
    if (ReferenceEquals(dateOfBirth1, dateOfBirth2))
    {
        return 0;
    {
    else if (ReferenceEquals(dateOfBirth1, null))
    {
        return -1;
    }
    else if (ReferenceEquals(dateOfBirth2, null))
    {
        return 1;
    }
    if (dateOfBirth1._value < dateOfBirth2._value)
    {
        return -1;
    }
    else if (dateOfBirth1._value > dateOfBirth2._value)
    {
        return 1;
    }
    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

I agree what the other two answers. This class doesn't add anything that would make it more useful than the ordinary DateTime.


Other issues that weren't addressed yet are...

public DateTime Value
{
    get { return _value; }
}

Introducing this property makes working with the DateOfBrith just more confusing. It gives the user a direct access to the underlaying data. This undermines the entire purpose of encapsulating and hiding the date-of-birth as a pure DateTime.


But not only that. It also uses pretty unhelpful exceptions:

public DateOfBirth(DateTime dateOfBirth)
{
    if(dateOfBirth == DateTime.MinValue)
        throw new ArgumentException("Invalid value.", "DateOfBirth");
    this._value= dateOfBirth;
}

The ArgumentException already means that some value was invalid so writing it as a message doesn't help. I expect you to tell me why it was invalid and what should I use to make it valid again. The other argument is also misleading. The argument name is dateOfBirth and not DateOfBirth. You should be using nameof() here anyway. Nobody uses such harcoded strings anymore.


public int CompareTo(DateOfBirth other)
{
    if (other != null)
        return this._value.CompareTo(other._value);
    else
        throw new ArgumentNullException("DateOfBirth");
}

The other exception is even worse because it does not only change the case like the first one but it uses an entirely different name! It should be other and not DateOfBirth.

But as @Heslacher said, this shouldn't be even throwing any exceptions.


(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)

I find the names of the arguments a little bit weird. By convention they are usually named left and right for where they come from or x and y and not some name+number.


The implementation is also inconsitent. You redirect all operators to Comparison but reinvent the wheel here

public bool Equals(DateOfBirth other)
{
    if (ReferenceEquals(other, null))
        return false;
    return _value == other._value;
}

by doing exactly the same equality check as you already did earlier.

\$\endgroup\$
  • \$\begingroup\$ Thanks. If I don't expose a datetime value to the calling code then how do they know what the value encapsulated is? \$\endgroup\$ – w0051977 Feb 3 '18 at 13:24
  • \$\begingroup\$ @w0051977 that's the whole point. You either should have explicitly cast it to DateTime or use your class everywhere. It would be quite pointless to use the Value because then you can easily avoid the entire encapsulation and you can use a DateTime instead without loosing anything. \$\endgroup\$ – t3chb0t Feb 3 '18 at 13:27
  • \$\begingroup\$ Thanks. I originally asked the question here: stackoverflow.com/questions/48582699/…, but then deleted that question to ask here instead. I have undeleted the question. Please see Pieter Witvoet comment. \$\endgroup\$ – w0051977 Feb 3 '18 at 13:34
  • \$\begingroup\$ @w0051977: with 'observing the actual date' I meant being able to inspect its year, month and day (that's what a date is, right?), not exposing an internal implementation detail (its DateTime field). \$\endgroup\$ – Pieter Witvoet Feb 3 '18 at 15:53

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.