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; }
}
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Jul 1, 2015 at 6:17
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Why isn't this written as the Equals() method of ExpiryDates? \$\endgroup\$ Commented Jul 1, 2015 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\$ Commented Jul 1, 2015 at 6:39
  • 2
    \$\begingroup\$ This code wouldn't be much different, but the code that uses the comparison would be more idiomatic. \$\endgroup\$ Commented Jul 1, 2015 at 6:40
  • \$\begingroup\$ @200_success : good point! Moving this code to Equals() \$\endgroup\$ Commented Jul 1, 2015 at 6:41

3 Answers 3

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);
}
answered Jul 1, 2015 at 7:41
\$\endgroup\$
0
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?

answered Jul 1, 2015 at 6:38
\$\endgroup\$
0
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.

answered Jul 1, 2015 at 7:33
\$\endgroup\$
4
  • \$\begingroup\$ Just a question, why "input parameter should be named using camelCase casing" ? where do this rule come from? \$\endgroup\$ Commented Jul 2, 2015 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\$ Commented Jul 2, 2015 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\$ Commented Jul 2, 2015 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\$ Commented Jul 2, 2015 at 12:12

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.