3
\$\begingroup\$

I'm writing a C# extension-method that converts any object to a type. E.g. the method shall convert a string with value "y" to a true-boolean - so there will be some specific castings like bools, chars, DateTimes,...

Here are some examples of my target:

int number = 3;
string stringNumber = number.To<string>();

stringNumber now should be a string with value equals to "3". (Yeah, there's already the ToString(), it's just an example) another one:

string myDateString = "Mon, 27 Jun 2007 12:34:56 CET";
DateTime myDate = myDateString.To<DateTime>();

myDate now should be a DateTime-object with value equals to a Date on 27th June of 2007 on time 12:34:56.

So, what I did is, I wrote following method:

public static T To<T>(this object @this)
{
    CastTo castedValue = null;

    if (typeof(T) == typeof(DateTime) || typeof(T) == typeof(DateTime?))
    {
        DateTime? result = null;

        if (@this is string actualStringValue)
            result = _tryCastStringToDateTime(actualStringValue);

        if (result != null)
            castedValue = new CastTo<DateTime> {Value = result.Value};
    }

    else if (typeof(T) == typeof(bool) || typeof(T) == typeof(bool?))
    {
        bool? result = null;

        if (@this is string actualStringValue)
            result = _tryCastStringToBoolean(actualStringValue);

        else if (@this is int actualIntegerValue)
            result = _tryCastIntegerToBoolean(actualIntegerValue);

        if (result != null)
            castedValue = new CastTo<bool> {Value = result.Value};
    }

    if (castedValue == null || castedValue.IsInitial)
        return (T) Convert.ChangeType(@this, typeof(T));

    return ((CastTo<T>) castedValue).Value;
}

those _tryCast....-methods simply cast an object to the specific type.

Here are my classes CastTo and CastTo<T>.

internal abstract class CastTo
{
    public abstract bool IsInitial { get; }

    public object Value { get; set; }
}

internal class CastTo<T> : CastTo
{
    private T _value;

    public override bool IsInitial => EqualityComparer<T>.Default.Equals(Value, default(T));

    public new T Value
    {
        get => _value;
        set
        {
            if (!EqualityComparer<T>.Default.Equals(value, default(T)))
                _value = value;
        }
    }
}

Are there any issues you'd suggest me to change?

  • Code-Style?
  • Performance?
  • Bad Practice?
  • Someone said, this is a misuse of generics. Is it really?
\$\endgroup\$
  • \$\begingroup\$ What is the use-case for this? It's possible that a better solution exists at a slightly higher level; this task as presented is inherently dodgy (but not a misuse of generics as far as I'm concerned). \$\endgroup\$ – VisualMelon Aug 2 '18 at 11:33
  • \$\begingroup\$ @VisualMelon I don't always want to write something like int.TryParse(value, out int x) - value.To<int>() is shorter and in my opinion more beautiful. The usecase for me is only having a more readable code. What you mean by dodgy? \$\endgroup\$ – Matthias Burger Aug 2 '18 at 11:40
  • 4
    \$\begingroup\$ If that's your use-case, then perhaps it is an abuse of generics... it's dodgy because it's a simple operation, but is liable to fail and (because it doesn't discriminate the type of @this) is easy to misapply. As an alternative to int.Parse() and others, you could write dedicated explicitly-typed extension methods (e.g. ToInt(this string @this), which would be harder to misapply, and introduce no more complexity into the implementation, while maintaining type-safety. \$\endgroup\$ – VisualMelon Aug 2 '18 at 11:53
  • 1
    \$\begingroup\$ I assume this is meant for (user) input parsing and user-friendly formatting? How do you determine which formats you need to handle? \$\endgroup\$ – Pieter Witvoet Aug 2 '18 at 12:28
  • \$\begingroup\$ @PieterWitvoet no, not really. Just general casting. My Application gets E-Mails and it tries to convert the date of the email to a DateTime, for storing it in the database (for example) - sometimes its RFC1123, sometimes not. In MVC there are some default variables defined by microsoft as a string. i know my application only stores ints as string in them. my API only uses ints. aaand for sure, a user sometimes needs to put a text in a field, maybe I need to parse it - could be. This is more a small library of extension-methods for my webapplications. \$\endgroup\$ – Matthias Burger Aug 2 '18 at 12:42
6
\$\begingroup\$

This design looks to me like a massive violation of KISS. For a start, I don't see the benefit of CastTo and CastTo<T>. It seems to me that they could be eliminated by tweaking the wrapped conversion methods to work more like int.TryParse with two return values: one to say whether the conversion succeeded, and the other to tell you what the result was. This doesn't necessarily need to use the same pattern of bool return value and out parameter for the result: it could use Tuple and C# 7's tuple unwrapping, for example. But it would simplify the 57 lines of code in the question to 19:

public static T To<T>(this object @this)
{
    if (typeof(T) == typeof(DateTime) || typeof(T) == typeof(DateTime?))
    {
        if (@this is string actualStringValue && _tryCastStringToDateTime(actualStringValue, out var result))
            return result;
    }

    else if (typeof(T) == typeof(bool) || typeof(T) == typeof(bool?))
    {
        if (@this is string actualStringValue && _tryCastStringToBoolean(actualStringValue, out var result))
            return result;

        else if (@this is int actualIntegerValue && _tryCastIntegerToBoolean(actualIntegerValue, out result))
            return result;
    }

    return (T) Convert.ChangeType(@this, typeof(T));
}

However, that code still raises red flags. There are situations where you have no choice but to use == typeof(...), but they are rare and mainly related to serialisation. Normally it's a sign that the code should be refactored. This method seems to be actively fighting against the type system. So I would agree completely with the person who told you that it's an abuse of generics. Consider also that if you expose the parse methods as

public static bool ToBool(this string str) { ... }
public static bool ToBool(this int i) { ... }
public static bool ToBool(this object obj)
{
    if (obj is string str) return ToBool(str);
    if (obj is int i) return ToBool(i);
    return (bool)Convert.ChangeType(obj, typeof(bool));
}

then in many actual use cases the compiler will be able to bind directly to the more specific method and skip the runtime checks.

(NB Code not tested, may have compile-time errors, but the principle is the important thing).

\$\endgroup\$
  • \$\begingroup\$ I understand what you mean. Nevertheless, I don't think everybody understands (or wants to understand) what I wanted to achieve and why. So I guess, I'll remove my To<T>-extension-method and keep using the Convert.ChangeType and TryParse-methods. \$\endgroup\$ – Matthias Burger Aug 2 '18 at 18:54
5
\$\begingroup\$

I don't think the To<T> API by itself is wrong and I also don't think this is a misuse of generics. There is no difference whether it's called

Convert(object value, Type targetType)

or just

To<TTarget>(this object value)

The result will be the same. All it does it to provide a simpler API if the type is known at compile time.


But what I don't like is the internal implementation of it because it's not extendable at all. You cannot easily add other conversions and you cannot easily test it.

To<T> should actually be only a convenience method that uses a more sophisticated framework to do the work.

It should also have more overloads for specifying the format string and IFormatProvider. This would be necessary to parse some non-default-format dates.

You can take a look at this great answer by @Dmitry Nogin where he shows how such a framework could look like. (and in fact I use it (with a couple of changes) in all my applications).

\$\endgroup\$
  • \$\begingroup\$ Somehow, I like this answer more... there are some things I didn't think about, that makes my conversion-function bad. \$\endgroup\$ – Matthias Burger Aug 3 '18 at 9:02

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.