Closures in C#, necessary or not?

Question

I've written JavaScript for a while, but lately I've been writing a lot of C#. I wanted to write a method that could take the column name of a DataTable and return an average for those values. Having never written closures in C#, but being comfortable with them in JavaScript, I came up with this:

public static double GetColumnAverage(DataTable dt,string columnName)
{
    Func<DataTable, double> getAverage = (t) => t.AsEnumerable().Where(x => x[columnName] != DBNull.Value)
                                                                        .Average(x => x.Field<dynamic>(columnName));
    bool isNumeric = true;
    foreach (DataRow row in dt.Rows)
    {
        var type = row[columnName].GetType();
        //not all numeric data types, but close enough for this
        isNumeric = type == typeof(int) || type == typeof(double) || type == typeof(float) || type == typeof(uint)
            || type == typeof(byte);
        if (isNumeric ==false)
        {
            throw new Exception();
        }
    }
    return getAverage(dt);
}

From this I learned that the parameters passed to an inner delegate are matched to the outer delegate, so Intellisense picked up the t parameter as a DataTable (not sure about implications, but neat). Cool. My initial thought was to create a private helper method IsNumeric that would return a bool if the value in the DataRow was something we could average.

I know that I could probably improve this a little, but I'm wondering if there's any practical reason to using a closure here, or if there are any potential pitfalls that might make it a bad idea?

Answer 1

I would recommend reading this article by Eric Lippert

You'll definitally want an extension method for this... let me hit the google real quick.... Using some inspired code from this SO post I wrote this.

static class Extensions
{
    private const HashSet<Type> NumericTypes = new HashSet<Type> 
        {typeof(decimal), typeof(byte), typeof(sbyte), typeof(short), typeof(ushort),
        typeof(int),typeof(uint), typeof(long), typeof(ulong), typeof(float), typeof(double)};

    /// <summary>Determines whether the specified object is numeric.</summary>
    public static bool IsNumeric(this object obj) { return NumericTypes.Contains(obj.GetType()); }
}

Now in your code you can just say

if(!row[columnName].IsNumeric())
    throw new Exception();

However, now I will review your code in general.

1. You should not throw Exception, you should always be specific with your exception throwing. In this case I probably would throw an ArgumentException, but really this seems like a specific case where you should define your own.

2. I'm a bit confused by your declaring isNumeric outside the loop, when you don't use it outside that scope.

3. instead of saying if (isNumeric == false) just say if(!isNumeric)

4. I don't see why you nested this function inside your loop. its not taking full advantage of the fact that it is nested, and could just aswell be a function outside. Plus why declare it if you might be throwing an exception.. might as well wait. And while you're waiting, lets just take this one liner and make it a statement instead. (My thoughts jumped around there a bit)

In the end your code could just look like.

public static double GetColumnAverage(DataTable dt, string columnName)
{
    foreach (DataRow row in dt.Rows)
        if (!row[columnName].IsNumeric())
            throw new ArgumentException("-Something meaningful here-");
    return dt.AsEnumerable().Where(x => x[columnName] != DBNull.Value).Average(x => x.Field<dynamic>(columnName));
}