6
\$\begingroup\$

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?

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

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));
}
\$\endgroup\$
6
  • \$\begingroup\$ Thanks for the contribution. The reason why I declared the lambda at the top of the method is simply because that's what JavaScript does: it hoist the inner function to the top of the containing function. Secondly (again, this is coming from more of a JS background) I feel that letting someone else who might be reading your code that there's a closure as soon as possible helps readability. Personally, I don't think having an IsNumeric extension method is the best bet, but I was exploring some new ground and wanted to test the waters a bit :) \$\endgroup\$ Mar 19, 2014 at 21:56
  • \$\begingroup\$ @wootscootinboogie you could make it a normal method too if you wanted to (isNumeric), but Extension methods are awesome, so why not :D. you can declare the lambda outside your function, or inside. But in your case there is no reason to even have a lambda. Might as well just run the code \$\endgroup\$
    – BenVlodgi
    Mar 19, 2014 at 22:06
  • \$\begingroup\$ agreed, I don't think the lambda here serves any real useful function, and probably makes the code less readable. \$\endgroup\$ Mar 19, 2014 at 22:12
  • \$\begingroup\$ 1. How is closing over the loop variable relevant? The posted code doesn't close over row. \$\endgroup\$
    – svick
    Mar 20, 2014 at 0:08
  • \$\begingroup\$ 2. You can't have const fields with complex values, I think you meant readonly. \$\endgroup\$
    – svick
    Mar 20, 2014 at 0:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.