HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcsharpMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
genericmethodreturnattemptextensionparsevaluedefaultwillthat

Problem

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());
        Console.WriteLine(("false").ParseOrDefault());
        Console.WriteLine(("23").ParseOrDefault());
        Console.WriteLine(("52").ParseOrDefault());
        Console.WriteLine(("4/22/1989").ParseOrDefault());
        Console.WriteLine(("4/22/2016").ParseOrDefault());
    }
}

public static class DataProcessAbstractionExtensions
{
    public static T ParseOrDefault(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);
    }
}

Solution

-
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.

Code Snippets

public static bool? ToBoolOrNull(this string str) 
{
    bool result;
    return Boolean.TryParse(str, out result) ? result : (bool?) null;
}

Context

StackExchange Code Review Q#126405, answer score: 7

Revisions (0)

No revisions yet.