Comparing two objects having two nullable date fields

Question

ExpiryDates has two properties PSL_ExpiryDate and MNL_ExpiryDate of type nullable date. I am trying to compare two objects for having same or different values.

Is there a cleaner way to do the same as the code below?

private bool SameValues(ExpiryDates ExpiryDates1, ExpiryDates ExpiryDates2)
    {
        //Assume they are the same value and the look for differences
        bool result = true;

        if (ExpiryDates1.PSL_ExpiryDate.HasValue != ExpiryDates2.PSL_ExpiryDate.HasValue)
        {
            result = false;
        }

        if (ExpiryDates1.MNL_ExpiryDate.HasValue != ExpiryDates2.MNL_ExpiryDate.HasValue)
        {
            result = false;
        }

        if ((ExpiryDates1.MNL_ExpiryDate != null) && (ExpiryDates2.MNL_ExpiryDate != null))
            if (ExpiryDates1.MNL_ExpiryDate.Value != ExpiryDates2.MNL_ExpiryDate.Value)
                result = false;

        if ((ExpiryDates1.PSL_ExpiryDate != null) && (ExpiryDates2.PSL_ExpiryDate != null))
            if (ExpiryDates1.PSL_ExpiryDate.Value != ExpiryDates2.PSL_ExpiryDate.Value)
                result = false;

        return result;
    }

The ExpiryDate class looks like this:

public class ExpiryDates
{
    public DateTime? MNL_ExpiryDate { get; set; }
    public DateTime? PSL_ExpiryDate { get; set; }
}

Answer 1

Nullable<T> already does the hard work for you. Just override Equals on your ExpiryDates class (and GetHashCode):

public override bool Equals(object obj)
{
    var otherDates = obj as ExpiryDates;

    return otherDates != null 
        && MNL_ExpiryDate.Equals(otherDates.MNL_ExpiryDate) 
        && PSL_ExpiryDate.Equals(otherDates.PSL_ExpiryDate);
}

As pointed out by mjolka in a comment. It would be advisible to implement IEquatable<T> (IEquatable<ExpiryDates>) as well. See the documentation for why it's a useful thing to do.

Further edit... It does say in the comments for IEquatable<T> but you should additionally override == and != e.g.

public static bool operator ==(ExpiryDates left, ExpiryDates right)
{
    if (object.ReferenceEquals(left, null))
    {
        return object.ReferenceEquals(right, null);
    }
    return left.Equals(right);
}

Answer 2

When you already know the value to return, don't guard it in a local variable. Return it immediately. In this example many conditions are evaluated even after you already know the result. If you don't return immediately, you also risk changing the local variable later in the function by mistake.

It seems you're missing some conditions. If one of ExpiryDates1.MNL_ExpiryDate and ExpiryDates2.MNL_ExpiryDate is null but not both, the function will return true. The same goes for the other date pairs. Or is that intended?

Answer 3

From phone, therfor short answer

• you need to check both input parameter for null, otherwise you risk a NullReferenceException

• If HasValue is false then the nullable is null, so either check the HasValue property or check if the nullable is null, but don't mix both styles.

• input parameter shoul be named using camelCase casing.

• using braces {} for single command if statements will help you to make your code less error prone. Also this is discussed contraversially, mixing both styles is a code smell. Be consistent in the style you use.