Add operator overrides for public IComparable types, #683

[paulirwin]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Add operator overrides for public IComparable types

Fixes #683

Description

This is to resolve the Sonar warning about needing to override the comparison operators for any types that implement IComparable. We are only doing this here for the public types.

QualityQuery was missing Equals and GetHashCode, as well as had some possible NullReferenceExceptions, so those have been fixed as well.

[paulirwin]

This change caught a NRE bug in MutableValue 😄 Fixed.

[NightOwl888]

This is a draft PR, because I'm currently unsure about whether to override Equals and GetHashCode (and if so, how exactly to do that, i.e. what to compare) for QualityQuery. @NightOwl888 - please let me know your thoughts.

Well, being that they didn't override the default behavior (which is reference equality), a direct port would be to override each and then cascade the call to the base class.

---

But given the fact that the comparer uses queryID and it says "ID of this quality query" for a description, it would probably be best to use that to compare for equality/hash code. It would keep the equality and compare checks exactly in sync.

BTW - I noticed that the QualityQuery.CompareTo() implementation has an exception handler that we can remove because we can use int.TryParse() and fallback to string comparison if parsing either side returns false.

        public virtual int CompareTo(QualityQuery other)
        {
            try
            {
                // compare as ints when ids ints
                int n = int.Parse(queryID, CultureInfo.InvariantCulture);
                int nOther = int.Parse(other.queryID, CultureInfo.InvariantCulture);
                return n - nOther;
            }
            catch (Exception e) when (e.IsNumberFormatException())
            {
                // fall back to string comparison
                return queryID.CompareToOrdinal(other.queryID);
            }
        }

The constructor could also be improved by using guard clauses on queryID and nameValPairs since there will be NREs if either is passed null. If we do so, we don't need to change CompareTo() to account for the fact that queryID may be null (which it currently doesn't handle).

#nullable enable would catch this sort of thing.

[NightOwl888]

Thanks for the PR.

We are getting some build warnings in some types where Equals(object) and GetHashCode() have not been implemented.

Also, since these are all on nullable types, please use #nullable enable within the #region Operator overrides sections and #nullable restore prior to #endregion. We don't necessarily have to do this on the entire file in this PR.

See the inline comments.

[eladmarg]

there is a big performance penalty for using this inside try/catch block.
its much better to use this with TryParse and avoid this block for so hot-path function.

ok, i see it fixed. great

[paulirwin]

there is a big performance penalty for using this inside try/catch block. its much better to use this with TryParse and avoid this block for so hot-path function.

ok, i see it fixed. great

Sorry, what are you referring to?