6
\$\begingroup\$

I recently wrote this code as a more versatile stand-in for Convert.ChangeType. I have a nagging feeling that there's something I might be overlooking, or that there might be a more efficient algorithm for this.

    /// <summary>
    ///     Returns an object of type <typeparamref name="T"/> whose value is equivalent to that of the specified 
    ///     object.
    /// </summary>
    /// <typeparam name="T">
    ///     The output type.
    /// </typeparam>
    /// <param name="value">
    ///     An object that implements <see cref="IConvertible"/> or is <see cref="Nullable{T}"/> where the underlying
    ///     type implements <see cref="IConvertible"/>.
    /// </param>
    /// <returns>
    ///     An object whose type is <typeparamref name="T"/> and whose value is equivalent to <paramref name="value"/>.
    /// </returns>
    /// <exception cref="System.ArgumentException">
    ///     The specified value is not defined by the enumeration (when <typeparamref name="T"/> is an enum, or Nullable{T}
    ///     where the underlying type is an enum).
    /// </exception>
    /// <exception cref="System.InvalidCastException"
    /// <remarks>
    ///     This method works similarly to <see cref="Convert.ChangeType(object, Type)"/> with the addition of support
    ///     for enumerations and <see cref="Nullable{T}"/> where the underlying type is <see cref="IConvertible"/>.
    /// </remarks>
    internal static T ChangeType<T>(object value) {

        Type type = typeof(T);
        Type underlyingNullableType = Nullable.GetUnderlyingType(type);

        if ((underlyingNullableType ?? type).IsEnum) {

            // The specified type is an enum or Nullable{T} where T is an enum.

            T convertedEnum = (T)Enum.ToObject(underlyingNullableType ?? type, value);

            if (!Enum.IsDefined(underlyingNullableType ?? type, convertedEnum)) {
                throw new ArgumentException("The specified value is not defined by the enumeration.", "value");
            }

            return convertedEnum;
        } else if (type.IsValueType && underlyingNullableType == null) {

            // The specified type is a non-nullable value type.

            if (value == null || DBNull.Value.Equals(value)) {
                throw new InvalidCastException("Cannot convert a null value to a non-nullable type.");
            }

            return (T)Convert.ChangeType(value, type);
        }

        // The specified type is a reference type or Nullable{T} where T is not an enum.
        return (value == null || DBNull.Value.Equals(value)) ? default(T) : (T)Convert.ChangeType(value, underlyingNullableType ?? type);
    }
\$\endgroup\$
3
\$\begingroup\$

Doc Comments

Well done! I rarely see a single method so thoroughly documented with XML doc comments. There's a catch though. Be careful about just how much you do this. It can really obstruct the readability of the actual code. I count roughly 20 lines of documentations here. Is all of that really necessary? I don't think it is.

For example:

/// <typeparam name="T">
///     The output type.
/// </typeparam>

That's..... useless. It's obvious. Don't document the obvious.

Style

I don't know a C# dev on this site that doesn't prefer new line braces to the "Egyptian" style braces that you use. If you're working with others, I would recommend you stick with the "C# style", but really, it doesn't matter. You were 100% consistent and that is what really matters at the end of the day.

Null Coalescence

if ((underlyingNullableType ?? type).IsEnum) {

WTFs per Minute

I'm sorry, but wtf? How do you expect anyone to wrap their head around that?

Null Coalescence has a time and place. This isn't it, and it's all over the place in this code. It's seriously harming readability/understandability.

I'll be clear about it. There's nothing wrong with underlyingNullableType ?? type, but once you put that inside of an if statement and call a method on it... it's.... meaningless. It becomes completely ungrokkable.

\$\endgroup\$
  • 1
    \$\begingroup\$ I agreed and I think for example: Type currentType = (underlyingNullableType ?? type); - would be a good start. \$\endgroup\$ – t3chb0t Dec 21 '14 at 14:55
  • \$\begingroup\$ It would be a very good start I think @t3chb0t. \$\endgroup\$ – RubberDuck Dec 21 '14 at 15:06
4
\$\begingroup\$
/// <summary>
///     Returns an object of type <typeparamref name="T"/> whose value is equivalent to that of the specified 
///     object.
/// </summary>

...

/// <returns>
///     An object whose type is <typeparamref name="T"/> and whose value is equivalent to <paramref name="value"/>.
/// </returns>

I don't see any reason to repeat what was said in <summary> in <returns>. I wouldn't write <returns> here at all. (Unless you're writing a library with very high demands on documentation, like the .Net framework itself.)


underlyingNullableType ?? type

You're repeating this expression several times. You should probably extract it into a variable.


You should be consistent: either use if-else if-else and ignore the returns, or acknowledge the returns and use if-if-nothing.

\$\endgroup\$
2
\$\begingroup\$

Review

  • re-organize your code to avoid redundant code segments underlyingNullableType ?? type or DBNull.Value checks
  • I just found out you have a follow-up question: pitty I didn't notice it before :) That one is also review-worthy / can be improved

Bugs

Additional reason why you can't use Enum.IsDefined: suppose we have the following enum..

[Flags]
enum A : uint
{
    None = 0,
    X = 1,
    Y = 2
}

Conversion fails because of not defined, even though A.X | A.Y is perfectly valid.

Refactored code

To be honest, I had to completely rewrite the flow to get rid of the redundant code blocks. First check the edge case when value is null. Then extract a nonNullableType to continue to work with. Process the other edge case with Enum and its underlying type. Use the exiting API Convert.ChangeType to map the normal cases. I have made some inline comments to explain what I'm doing.

public static T ChangeType<T>(object value)
{
    var isNull = IsNull(value);
    var type = typeof(T);

    if (isNull)
    {
        if (!type.IsNullAssignable())
        {
            throw new InvalidCastException($"Cannot cast null to {type}");
        }

        // null-assignable types (reference types and nullable types) can deal with null
        return default;
    }

    // use this type from here on to avoid the redundant 'if nullable .. else ..'
    var nonNullableType = type.AsNonNullable();

    if (nonNullableType.IsEnum)
    {
        // convert the value to the underlying type of the enum and
        // convert that result to the enum
        var enumUnderlyingType = Enum.GetUnderlyingType(nonNullableType);
        var enumUnderlyingValue = Convert.ChangeType(value, enumUnderlyingType);
        return (T)Enum.ToObject(nonNullableType, enumUnderlyingValue);
    }

    // let .NET handle remaining convertions
    return (T)Convert.ChangeType(value, nonNullableType);
}

public static bool IsNull(object value)
{
    // - value == null uses the type's equality operator (usefull for Nullable)
    // - ReferenceEquals checks for actual null references
    // - DBNull is a special null value

    return value == null 
        || ReferenceEquals(null, value)
        || value is DBNull;
}

Helper class

argument checks are left out for brevity

public static class TypeExtension
{
    public static bool IsNullable(this Type type)
    {
        return type.IsGenericType && type.GetGenericTypeDefinition().Equals(typeof(Nullable<>));
    }

    public static bool IsNullAssignable(this Type type)
    {
        return IsNullable(type) || !type.IsValueType;
    }

    public static Type AsNonNullable(this Type type)
    {
        return type.IsNullable() ? Nullable.GetUnderlyingType(type) : type;
    }
}

Use Cases / Tests

[TestMethod]
public void TestConversions()
{
    // Positive tests

    var enumFlags = ChangeType<A>(A.X | A.Y);
    var enumFlagsUnderlying = ChangeType<A>(4);
    var enumFlagsUnderlyingDifferentType = ChangeType<A>(4d);
    var enumFlagsNullable = ChangeType<A?>(A.X | A.Y);
    var enumFlagsNullableNull = ChangeType<A?>(null);
    var enumFlagsNullableDBNull = ChangeType<A?>(DBNull.Value);
    var referenceType = ChangeType<RefType>(null);
    var valueType = ChangeType<ValType>(default(ValType));
    var valueTypeNullable = ChangeType<ValType?>(default(ValType));
    var valueTypeNullableNull = ChangeType<ValType?>(null);
    var enumTypeWithDifferentUnderlyingTypes = ChangeType<A>(B.Z);
    var enumTypeWithDifferentUnderlyingTypesWithoutConstant = ChangeType<A>(B.ZZ);

    // Negative tests

    Assert.ThrowsException<InvalidCastException>(() => ChangeType<ValType>(null));
}
\$\endgroup\$
1
\$\begingroup\$

You should be using the standard C# bracing style

if (Condition)
{
    //Operations
}

instead of the Java Style bracing you have in your code, it makes it read funny.

\$\endgroup\$
  • 5
    \$\begingroup\$ As a C# dev, I agree with this, but I will point out that it looks like OP was consistent. And really, consistency is more important than which style braces he chose. \$\endgroup\$ – RubberDuck Dec 20 '14 at 20:46
  • \$\begingroup\$ @RubberDuck I agree. \$\endgroup\$ – Malachi Dec 20 '14 at 20:47

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.