Generic extension method that will attempt to parse a string and return it's value or default(T)

Question

So I got sick of several things about the way TryParse works. I implemented a generic ParseOrDefault function using reflection. It appears to work as expected, but I'm not fool enough to say my code is bullet proof. What's wrong with it? If something is wrong with it, can it be fixed, or should this idea be tossed?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;

public class Program
{
    public static void Main()
    {

        Console.WriteLine(("true").ParseOrDefault<bool>());
        Console.WriteLine(("false").ParseOrDefault<bool>());
        Console.WriteLine(("23").ParseOrDefault<int>());
        Console.WriteLine(("52").ParseOrDefault<int>());
        Console.WriteLine(("4/22/1989").ParseOrDefault<DateTime>());
        Console.WriteLine(("4/22/2016").ParseOrDefault<DateTime>());
    }
}


public static class DataProcessAbstractionExtensions
{
    public static T ParseOrDefault<T>(this string Value)
    {
        T ReturnedValue = default(T);
        //Since C# doesn't impliment an IParsable interface to filter generically
        //Reflection must be used to determine if object is in fact parsable
        var type = typeof(T);
        var MethodInfo = type.GetMethods().FirstOrDefault(MI =>
          {
              ParameterInfo[] ArgInfo = MI.GetParameters();
              return (ArgInfo.Length == 2 && ArgInfo[0].ParameterType == typeof(string) && ArgInfo[1].IsOut && ArgInfo[1].ParameterType.GetElementType() == type);
          });
        if (MethodInfo != null && MethodInfo.IsStatic)
        {
            object[] Arguments = new object[] { Value, ReturnedValue };
            MethodInfo.Invoke(null, Arguments);
            return (T)Arguments[1];
        };
        return default(T);
    }
}

It would be nice if we have extension method specialization, same way to c++ template specialization. But again, I might have just opened a pandora's box... — Xiaoy312, Apr 22 at 8:08

Konrad Morawski · Accepted Answer · 2016-04-22 08:53:31Z

For starters, I feel inclined to go meta and question the design. One possible shortcoming of your method is that it falls back to the default value, which - for primitives - isn't null. So "blah blah" would evaluate to (int) 0 or (bool) false, and unlike TryParse, your method won't tell me if this result actually came from the input, or whether it only reflects the inability to convert it. This could lead to issues with invalid data getting swept under the rug, and as such it's sort of buggy by design (especially when handling dates).
The biggest no-no though is that you're not handling exceptions. That's quite a leap of faith when invoking a method by reflection, especially as it's only loosely identified by its signature. I wouldn't blindly trust the return type, or that the method we happened to find actually does what we hope it does.

Given that the name of the method promises to fall back to default if parsing is impossible, I would say not handling an exception breaks the principle of least surprise here.
var MethodInfo = type.GetMethods().FirstOrDefault(MI =>

How do you know the method you're looking for will always come up first on the list returned by GetMethods? If you only expect one, use SingleOrDefault. But FirstOrDefault indicates you consider the possibility that more than one method matches your given criteria. So, do we have some guarantee it would always be the first one in our way, or are we just "feeling lucky"? :) This looks pretty fragile to me.
This is subjective of course, but I have to say that since this method is actually less functional than TryParse and the implementation is rather brittle, I would personally veto this extension in a peer review, since I don't believe whatever readability improvement it brings to the table justifies the trade-offs.

I think this is a textbook example of what Jon Skeet calls evil code. It's not wrong as in "doesn't work", it's sort of clever, its "magic" can even have some appeal to it, but it's fundamentally unclear and dangerous. I highly recommend this talk: https://youtu.be/lGbQiguuUGc.
If you really want such syntactic sugar, I'd ditch generics and reflection-based approach, and replace it with hardcoded extensions for bool, DateTime, int - come on, it's not like there's dozens of use-cases anyway. Oh, and have them return a Nullable to distinguish between input actually converted to the default value, and not converted at all. Like so:
```
public static bool? ToBoolOrNull(this string str) 
{
    bool result;
    return Boolean.TryParse(str, out result) ? result : (bool?) null;
}
```
And analogically for these few other types.

PS. Note that in C# lower-case names should be used for parameters and variables - so it would be this string value, methodInfo, mi etc. And I believe you don't really need ReturnedValue.

Ouch! But all good points. Excellent presentation, helps shed some of the light I have committed in the name of "I are smart". I went ahead and followed your suggestion number 5 (One-lining my parsing really does add readability). Thank you :) — Sidney, Apr 22 at 9:11
I understand you very well - all software devs, or at least all good ones, have this conflict between ther inner artist, who goes "be bold / push this language to the limits / wouldn't it be cool if...?", and this square engineer who insists "do not... play it safe... keep it down...", and I do get a bit sad every time I have to agree with the latter voice and give the shininess up :) But this talk by Skeet was one of my eye-openers: I think it was an important piece of the puzzle for growing from (clever) junior to senior. Thanks for accepting my answer, harsh as it may have been :) — Konrad Morawski, Apr 22 at 9:50

Xiaoy312 · Answer 2 · 2016-04-22 07:49:09Z

You don't need to pull every single method and compare Most of parsable type will have these signatures :

public static T T::Parse(string);
public static bool T::TryParse(string, out T);

You can look specifically for them, instead of comparing the argument list of each method, which is really costly.

public static T ParseOrDefault<T>(this string value)
{
    var type = typeof(T);
    //Since C# doesn't impliment an IParsable interface to filter generically
    //Reflection must be used to determine if object is in fact parsable
    var tryParse = type.GetMethod("TryParse",
        BindingFlags.Public | BindingFlags.Static,
        null,
        new [] { typeof(string), type.MakeByRefType() },
        null);
    if (tryParse == null)
        return default(T);

    object[] args = { value, null };
    if ((bool)tryParse.Invoke(null, args))
        return (T)args[1];

    return default(T);
}

However, I still think using type-check and invoking TryParse directly would be faster.

I think it's a bad idea (for production code), no matter how much lipstick we put on that pig — Konrad Morawski, Apr 22 at 8:46
True. This extension doesn't seem to be that much helpful, for the trouble it brings. And Parse<int> isn't much better than ParseInt, in fact it even takes few more keystrokes. — Xiaoy312, Apr 22 at 9:18

asked	1 month ago
viewed	335 times
active	1 month ago

current community

your communities

more stack exchange communities

Generic extension method that will attempt to parse a string and return it's value or default(T)

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged c# reflection duck-typing or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Generic extension method that will attempt to parse a string and return it's value or default(T)

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# reflection duck-typing or ask your own question.

Related

Hot Network Questions