4
\$\begingroup\$

I'm writing a chart.js port for c# blazor (you can find it here) using C# 8.0 with .NET Core 3 (latest preview).
In chart.js there is this thing called indexable options. It let's you use either a single value or an array and handles it accordingly.
Because I want the port to be as type-safe as possible, I implemented a wrapper which allows you to store a value which either represents a single value or an array of values.

Points that might need (the most) attention:

Equality

This is the main thing I'd like feedback on. I've implemented IEquatable and tried to write "smart" equality checks which use the most appropriate method to compare but since I'm not an expert, I don't expect them to be perfect :)

Conversion

Because I have implemented two implicit and two explicit conversions, which are vital for the coding experience when using this struct, it's important to me that those are correct.

Summaries

This isn't as important as the other factors but I still think it's nice to write good summaries, especially considering this is a library.
I tried to keep it consistent and mimic the summaries of built-in classes like string where it makes sense. Again, it's not as important but if you see something and think "you don't do that usually", I'd love to hear what you'd do instead.

Other

Improving coding style (and other "basic" stuff) is of course always good so don't hold back on that if anything bugs you.

I'd also like to mention my thoughts on a few design choices because maybe I was wrong before I even started coding this class:

  • Use struct instead of class because it has value semantics.
  • Implement IEquatable because it's a struct and therefore avoid unnecessary boxing.
  • Keep the Value-property public simply because there's no reason to hide it.
  • Use an array instead of e.g. an IEnumerable because I want the values to already be present when the instance is serialized. This way there is definitely no downtime wasted on e.g. database access when I try to serialize the instance.
/// <summary>
/// Represents an object that can be either a single value or an array of values. This is used for typesafe js-interop.
/// </summary>
/// <typeparam name="T">The type of data this <see cref="IndexableOption{T}"/> is supposed to hold.</typeparam>
public struct IndexableOption<T> : IEquatable<IndexableOption<T>>
{
    /// <summary>
    /// The compile-time name of the property which stores the wrapped value. This is used internally for serialization.
    /// </summary>
    internal const string PropertyName = nameof(Value);

    /// <summary>
    /// The actual value represented by this instance.
    /// </summary>
    public object Value { get; }

    /// <summary>
    /// Gets the value indicating whether the option wrapped in this <see cref="IndexableOption{T}"/> is indexed. 
    /// <para>True if the wrapped value represents an array of <typeparamref name="T"/>, false if it represents a single value of <typeparamref name="T"/>.</para>
    /// </summary>
    public bool IsIndexed { get; }

    /// <summary>
    /// Creates a new instance of <see cref="IndexableOption{T}"/> which represents a single value.
    /// </summary>
    /// <param name="singleValue">The single value this <see cref="IndexableOption{T}"/> should represent.</param>
    public IndexableOption(T singleValue)
    {
        Value = singleValue ?? throw new ArgumentNullException(nameof(singleValue));
        IsIndexed = false;
    }

    /// <summary>
    /// Creates a new instance of <see cref="IndexableOption{T}"/> which represents an array of values.
    /// </summary>
    /// <param name="indexedValues">The array of values this <see cref="IndexableOption{T}"/> should represent.</param>
    public IndexableOption(T[] indexedValues)
    {
        Value = indexedValues ?? throw new ArgumentNullException(nameof(indexedValues));
        IsIndexed = true;
    }

    /// <summary>
    /// Implicitly wraps a single value of <typeparamref name="T"/> to a new instance of <see cref="IndexableOption{T}"/>.
    /// </summary>
    /// <param name="singleValue">The single value to wrap</param>
    public static implicit operator IndexableOption<T>(T singleValue) => new IndexableOption<T>(singleValue);

    /// <summary>
    /// Implicitly wraps an array of values of <typeparamref name="T"/> to a new instance of <see cref="IndexableOption{T}"/>.
    /// </summary>
    /// <param name="indexedValues">The array of values to wrap</param>
    public static implicit operator IndexableOption<T>(T[] indexedValues) => new IndexableOption<T>(indexedValues);

    /// <summary>
    /// Explicitly unwraps an <see cref="IndexableOption{T}"/> to a single value.
    /// <para>If this instance represents an array of values instead of a single value, an <see cref="InvalidCastException"/> will be thrown.</para>
    /// </summary>
    /// <param name="wrappedValue">The wrapped single value</param>
    public static explicit operator T(IndexableOption<T> wrappedValue)
    {
        if (wrappedValue.IsIndexed)
            throw new InvalidCastException("This instance represents an array of values and can't be converted to a single value.");

        return (T)wrappedValue.Value;
    }

    /// <summary>
    /// Explicitly unwraps an <see cref="IndexableOption{T}"/> to an array of values.
    /// <para>If this instance represents a single value instead of an array of values, an <see cref="InvalidCastException"/> will be thrown.</para>
    /// </summary>
    /// <param name="wrappedValue">The wrapped array of values</param>
    public static explicit operator T[](IndexableOption<T> wrappedValue)
    {
        if (!wrappedValue.IsIndexed)
            throw new InvalidCastException("This instance represents a single value and can't be converted to an array of values.");

        return (T[])wrappedValue.Value;
    }

    /// <summary>
    /// Determines whether the specified <see cref="IndexableOption{T}"/> instance is considered equal to the current instance.
    /// </summary>
    /// <param name="other">The <see cref="IndexableOption{T}"/> to compare with.</param>
    /// <returns>true if the objects are considered equal; otherwise, false.</returns>
    public bool Equals(IndexableOption<T> other)
    {
        if (IsIndexed != other.IsIndexed) return false;

        if (IsIndexed)
        {
            return EqualityComparer<T[]>.Default.Equals((T[])Value, (T[])other.Value);
        }
        else
        {
            return EqualityComparer<T>.Default.Equals((T)Value, (T)other.Value);
        }
    }

    /// <summary>
    /// Determines whether the specified object instance is considered equal to the current instance.
    /// </summary>
    /// <param name="obj">The object to compare with.</param>
    /// <returns>true if the objects are considered equal; otherwise, false.</returns>
    public override bool Equals(object obj)
    {
        // an indexable option cannot store null
        if (obj == null) return false;

        if (obj is IndexableOption<T> option)
        {
            return Equals(option);
        }
        else
        {
            return Value.Equals(obj);
        }
    }

    /// <summary>
    /// Returns the hash of the underlying object.
    /// </summary>
    /// <returns>The hash of the underlying object.</returns>
    public override int GetHashCode()
    {
        return -1937169414 + Value.GetHashCode();
    }

    /// <summary>
    /// Determines whether two specified <see cref="IndexableOption{T}"/> instances contain the same value.
    /// </summary>
    /// <param name="a">The first <see cref="IndexableOption{T}"/> to compare</param>
    /// <param name="b">The second <see cref="IndexableOption{T}"/> to compare</param>
    /// <returns>true if the value of a is the same as the value of b; otherwise, false.</returns>
    public static bool operator ==(IndexableOption<T> a, IndexableOption<T> b) => a.Equals(b);

    /// <summary>
    /// Determines whether two specified <see cref="IndexableOption{T}"/> instances contain different values.
    /// </summary>
    /// <param name="a">The first <see cref="IndexableOption{T}"/> to compare</param>
    /// <param name="b">The second <see cref="IndexableOption{T}"/> to compare</param>
    /// <returns>true if the value of a is different from the value of b; otherwise, false.</returns>
    public static bool operator !=(IndexableOption<T> a, IndexableOption<T> b) => !(a == b);
}
\$\endgroup\$
3
\$\begingroup\$

Compiler error Unless you're using some experimental preview C# version, your code doesn't compile as this line:

Value = singleValue ?? throw new ArgumentNullException(nameof(singleValue));

yields an error

CS0019 Operator '??' cannot be applied to operands of type 'T' and ''

You would need to add a constraint for that like: where T : class


Nested options This operator produces nested IndexableOptions:

public static implicit operator IndexableOption<T>(T singleValue) => new IndexableOption<T>(singleValue);

This means when you do this:

var x = (object)new IndexableOption<object>();
IndexableOption<object> y = x;

you'll end up with x nested inside y. I find this operator should check whether T is already of type IndexableOption<T> to prevent nested options.

\$\endgroup\$
  • \$\begingroup\$ My bad I forgot to mention that I'm using C# 8 with .net core 3 (latest preview). Good catch about the nested options, entirely missed that, thanks. \$\endgroup\$ – Joelius Aug 30 at 5:57
  • \$\begingroup\$ @t3chb0t I'm just reflecting my edit. Did I invalidate your answer by including the targetted framework in the question? \$\endgroup\$ – dfhwze Aug 30 at 12:05
  • 1
    \$\begingroup\$ @dfhwze in a certain way you did, indeed but you're forgiven; let's say it was late :-] \$\endgroup\$ – t3chb0t Aug 30 at 12:11
3
\$\begingroup\$

It's a bit counter intuitive you store Value as Object if the class IndexableOption<T> is generic. I understand you are trying to find a common type to store both T as T[]. However, if T : struct then what happens in case Value is T is a thing called boxing*.

From reference source:

Performance

In relation to simple assignments, boxing and unboxing are computationally expensive processes. When a value type is boxed, a new object must be allocated and constructed. To a lesser degree, the cast required for unboxing is also expensive computationally.

For instance, in Equals there is an expensive unboxing:

if (IsIndexed)
{
    return EqualityComparer<T[]>.Default.Equals((T[])Value, (T[])other.Value);
}
else
{
    //                                        unboxing#1   unboxing#2
    return EqualityComparer<T>.Default.Equals((T)Value, (T)other.Value);
}

I don't think it's worth storing both T as T[] in the same property. Why don't you use 2 properties instead and use the one associated with the value of IsIndexed?

public T Value { get; }
public T[] IndexedValue { get; }

Equals refactored:

if (IsIndexed)
{
    return EqualityComparer<T[]>.Default.Equals(IndexedValue , other.IndexedValue );
}
else
{
    return EqualityComparer<T>.Default.Equals(Value, other.Value);
}

One other thing, you may not like that the consumer has to call IsIndexed in order to decide to use Value or IndexedValue. If you really must, you can shield both properties from public access and still decide to use some boxing. But at least, internally, no redundant boxing/unboxing takes place.

protected T Value { get; }
protected T[] IndexedValue { get; }

public object ValueRef => IsIndexed ? IndexedValue : Value;

Or you may want to turn the logic around without using boxing:

public T[] ValueRef => IsIndexed ? IndexedValue : new[] { Value };

Which makes me wonder, perhaps you should only store T[] and return either the array or its sole element. This should avoid most recurring if-statements as well. I also think you should only use EqualityComparer<T>, not EqualityComparer<T[]>, but that has to be verified.


Footnote:

  • Boxing is the process of converting a value type to the type object or to any interface type implemented by this value type
\$\endgroup\$
  • 1
    \$\begingroup\$ Good stuff, thanks. As far as my library goes the consumer doesn't actually have to get the stored value, it's just serialized for the js-interop. With that in mind I'm thinking about using the two properties, make them internal and not even exposing any way of getting the value back. Is that a bad idea? I'm thinking it might be bad since it will still be a mutable type because you can still change the value that was stored, even if you can't get it back after creation by storing a reference first (for reference types and the array). \$\endgroup\$ – Joelius Aug 30 at 6:03
  • \$\begingroup\$ Also, about only using an array to store the value, I don't think I should do that. The reason is simply that an array with one value and one value alone has to and will be handled differently. What I could do of course is use IsIndexed and then decide if I should use an array or only the first element but that seems more confusing for no actual benefits (or is there something I'm missing which is worse about two properties?) \$\endgroup\$ – Joelius Aug 30 at 6:08
  • \$\begingroup\$ I don't think you are missing something, let's wait and see if other reviewers come up with alternative ideas. \$\endgroup\$ – dfhwze Aug 30 at 6:10
  • \$\begingroup\$ Okay. About the EqualityComparer<T>, not EqualityComparer<T[]>, you can't actually use EqualityComparer<T> for T[] because the Equals method will only accept two elements of T, not T[]. \$\endgroup\$ – Joelius Aug 30 at 10:22
  • 1
    \$\begingroup\$ I have asked on SO about it (it was a bad question though so I deleted it). The best thing to do probably is to use a == b (cheap reference check) and then go for Enumerable.SequenceEqual which will also use the correct equality comparer as you can see here. \$\endgroup\$ – Joelius Aug 30 at 16:35

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.