9
\$\begingroup\$

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; }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Why isn't this written as the Equals() method of ExpiryDates? \$\endgroup\$ – 200_success Jul 1 '15 at 6:34
  • \$\begingroup\$ @200_success : Suppose it was written in Equals method. Would the code be simpler or just as ugly as this? \$\endgroup\$ – Arjang Jul 1 '15 at 6:39
  • 2
    \$\begingroup\$ This code wouldn't be much different, but the code that uses the comparison would be more idiomatic. \$\endgroup\$ – 200_success Jul 1 '15 at 6:40
  • \$\begingroup\$ @200_success : good point! Moving this code to Equals() \$\endgroup\$ – Arjang Jul 1 '15 at 6:41
9
\$\begingroup\$

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);
}
\$\endgroup\$
5
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Just a question, why "input parameter should be named using camelCase casing" ? where do this rule come from? \$\endgroup\$ – Arjang Jul 2 '15 at 2:58
  • 1
    \$\begingroup\$ This comes from the NET naming guidelines. You can find a link to it in the [C#] tag wiki. \$\endgroup\$ – Heslacher Jul 2 '15 at 3:29
  • \$\begingroup\$ Are there any tools to go trough code and change the names to match the guidelines? This seems a task much more suited for machine than a human remembering bunch of rules cooked up by someone, something like intellisense squiggles to change the name. Doeas FX cop does this? \$\endgroup\$ – Arjang Jul 2 '15 at 5:05
  • 1
    \$\begingroup\$ Sorry for the delay. In the none free version of VS you have a Code Analysis tab in the project properties and a Analyze menue item to run code analysis. \$\endgroup\$ – Heslacher Jul 2 '15 at 12:12

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.