11
\$\begingroup\$

Given a boxed integer (with a boxed short, int, ...) I want to convert it to an unboxed, nullable type instead. It should be possible to convert non-matching types (for example: boxed int to short?) Can you please comment on:

using System;

public class Test
{
    public static void Main()
    {
        int? x = Unbox<int>((short)42);
        short? y = Unbox<short>(42);
    }

    static T? Unbox<T>(object x) where T : struct
    {
        return typeof(T) == x.GetType() ? (T)x : (T)Convert.ChangeType(x, typeof(T));
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Jan 10 '17 at 14:51
14
\$\begingroup\$

You should probably add IConvertible restriction on T, since it is used by Convert.ChangeType method.

Also this check: typeof(T) == x.GetType() looks like an overhead. Convert.ChangeType does nothing if types match. For example, check out Int32 sources:

int IConvertible.ToInt32(IFormatProvider provider) {
    return m_value;
}

So you might as well just check for null instead:

static T? Unbox<T>(object x) where T : struct, IConvertible
{
    if (x == null) return null;
    return (T)Convert.ChangeType(x, typeof(T));
}

Edit: as mentioned by sir t3chb0t, there are cases, where Convert.ChangeType can throw. So it might be a good idea to swallow exceptions inside your method and return null in those cases. But it also can be a very bad idea. It depends on use case, which you should carefully evaluate.

\$\endgroup\$
  • 1
    \$\begingroup\$ Note though, that ChangeType could have taken an IConvertible but chose to take object instead. It does support non-IConvertible "conversions". (See also stackoverflow.com/questions/3847997/…) \$\endgroup\$ – Micha Wiedenmann Jan 10 '17 at 14:38
  • 2
    \$\begingroup\$ should just make it return x == null ? null : (T)Convert.ChangeType(x, typeof(T)); one line it, or maybe better return x != null ? (T)Convert.ChangeType(x, typeof(T)) : null; \$\endgroup\$ – Malachi Jan 10 '17 at 14:57
  • \$\begingroup\$ @user127866 great observation. So if types match, ChangeType does not even hit actual IConvertible implementation. That's pretty weird design choice on Microsoft's part. \$\endgroup\$ – Nikita B Jan 11 '17 at 7:34
13
\$\begingroup\$

I'm not sure if you understand what boxing means because you use an int in the example.

Boxing is the process of converting a value type to the type object or to any interface type implemented by this value type.

and

When the CLR boxes a value type, it wraps the value inside a System.Object and stores it on the managed heap.

and

Unboxing extracts the value type from the object.

This would be unboxing. First box the int as object and then unbox it again.

object value = (short)42;
int? x = (int?)(short)value;
short? y = (short?)value;

I find this method is unecessary. You're better off with normal casting then with this and its overhead.

The problem with ChangeType is that

The ChangeType(Object, Type) method can convert a nullable type to another type. However, it cannot convert another type to a value of a nullable type, even if conversionType is the underlying type of the Nullable.To perform the conversion, you can use a casting operator (in C#) or a conversion function (in Visual Basic). The following example illustrates the conversion to and from a nullable type.

So this won't work:

short? z = (short?)Convert.ChangeType(value, typeof(short?));

but you

want to convert it to an unboxed, nullable type instead.

this means that if you still want to use the Unbox method the correct version of it should able to actually accept the int? as a parameter and return the default(T) rather then a null:

static T Unbox<T>(object x)
{
    if (x == null) return default(T);
    var nullableType = Nullable.GetUnderlyingType(typeof(T));
    return (T)Convert.ChangeType(x, nullableType ?? typeof(T));
}

but this is still casting the result with (T) so we're back at the beginning... this method is unnecessary.


Oh, and there is one more thing. ChangeType will blow if you try to convert int to short that exceedes the short.

It'll also hide the fact that you can loose precision when converting incompatibile types.

Consider this:

object value = (double)12.3;

short x = (short)(double)value;
int y = Unbox<int>(value);

To get the x you need to explicitly and deliberately cast it to short and see and know that you are converting double and may loose precision.

Getting the y via Unbox silently removes the fractional part.

\$\endgroup\$
  • \$\begingroup\$ While your statements are true, I am sorry that I failed in making the question more clear. a) Unboxed takes objects, "you use an int" is not correct/missunderstanding. b1) I did not intend that ConvertType would handle null, this was a bug in my question (which the rules of this site prevented me to edit) Sorry. I just use a null check to handle nulls now. b2) Normal casting requires multiple methods. In this case I don't want normal casting because I don't know the type to cast to. A generic method is what I want. c) I should have said, that I am not concerned with overflows. I am sorry. \$\endgroup\$ – Micha Wiedenmann Jan 11 '17 at 8:35
  • \$\begingroup\$ @user127866 thx for clarifying - next time you'll give more details and there won't by any misunderstandings ;-) \$\endgroup\$ – t3chb0t Jan 11 '17 at 8:39
  • \$\begingroup\$ @user127866 out of curiosity, why don't you know the data type? \$\endgroup\$ – t3chb0t Jan 11 '17 at 14:55
2
\$\begingroup\$

If you actually want to unbox the value and not simply convert it you can use the Expression API to build a function that does exactly that, in pseudo-code:

Func<object, TResult> = boxedValue => (TResult)(typeof(boxedValue))boxedValue;

To build the function you'll need a helper class to construct the delegate with the correct types:

static class UnboxerCache<TResult>
{
    private static readonly ConcurrentDictionary<Type, Func<object, TResult>> UnboxerLookup = new ConcurrentDictionary<Type, Func<object, TResult>>();

    public static Func<object, TResult> GetUnboxer(Type actualType)
    {
        return UnboxerLookup.GetOrAdd(actualType, CreateUnboxer);
    }

    private static Func<object, TResult> CreateUnboxer(Type actualType)
    {
        var boxedParameter = Expression.Parameter(typeof(object), "boxedValue");
        var unboxedValue = Expression.Unbox(boxedParameter, actualType);
        var convertedValue = Expression.Convert(unboxedValue, typeof(TResult));

        var unboxer = Expression.Lambda<Func<object, TResult>>(convertedValue, boxedParameter);

        return unboxer.Compile();
    }
}

Here we have a mapping from the actual type of the object to a delegate that can unbox and cast it to the result type. The construction of the delegate is pretty simple, convert the boxedValue to the actual type and then to the result type.

I would make the actual unboxer a separate class with that sole resposibility:

public static class Unbox
{
    public static TResult? To<TResult>(object boxedValue)
        where TResult : struct
    {
        if (boxedValue == null)
        {
            return null;
        }

        var actualType = boxedValue.GetType();
        var resultType = typeof(TResult);
        if (actualType == resultType)
        {
            return (TResult)boxedValue;
        }

        var unboxer = UnboxerCache<TResult>.GetUnboxer(actualType);
        return unboxer(boxedValue);
    }

    private static class UnboxerCache<TResult>
    {
        private static readonly ConcurrentDictionary<Type, Func<object, TResult>> UnboxerLookup = new ConcurrentDictionary<Type, Func<object, TResult>>();

        public static Func<object, TResult> GetUnboxer(Type actualType)
        {
            return UnboxerLookup.GetOrAdd(actualType, CreateUnboxer);
        }

        private static Func<object, TResult> CreateUnboxer(Type actualType)
        {
            var boxedParameter = Expression.Parameter(typeof(object), "boxedValue");
            var unboxedValue = Expression.Unbox(boxedParameter, actualType);
            var convertedValue = Expression.Convert(unboxedValue, typeof(TResult));

            var unboxer = Expression.Lambda<Func<object, TResult>>(convertedValue, boxedParameter);

            return unboxer.Compile();
        }
    }
}

Finally you should consider verifying that the type of the boxed value is a value type (boxedValue.GetType().IsValueType) and maybe raise an error if that isn't the case.

\$\endgroup\$
0
\$\begingroup\$

Unbox<T> is missing a null check. The call x.GetType() will fail if x = null.

static T? Unbox<T>(object x) where T : struct
{
    if (x == null)
        return null;

    return typeof(T) == x.GetType() ? (T)x : (T)Convert.ChangeType(x, typeof(T));
}
\$\endgroup\$

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.