patterncsharpMinor
Closures in C#, necessary or not?
Viewed 0 times
necessarynotclosures
Problem
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
From this I learned that the parameters passed to an inner delegate are matched to the outer delegate, so Intellisense picked up the
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?
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 getAverage = (t) => t.AsEnumerable().Where(x => x[columnName] != DBNull.Value)
.Average(x => x.Field(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?
Solution
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.
Now in your code you can just say
However, now I will review your code in general.
-
You should not throw
-
I'm a bit confused by your declaring
-
instead of saying
-
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.
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 NumericTypes = new HashSet
{typeof(decimal), typeof(byte), typeof(sbyte), typeof(short), typeof(ushort),
typeof(int),typeof(uint), typeof(long), typeof(ulong), typeof(float), typeof(double)};
/// Determines whether the specified object is numeric.
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.
-
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.-
I'm a bit confused by your declaring
isNumeric outside the loop, when you don't use it outside that scope.-
instead of saying
if (isNumeric == false) just say if(!isNumeric)-
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(columnName));
}Code Snippets
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()); }
}if(!row[columnName].IsNumeric())
throw new Exception();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));
}Context
StackExchange Code Review Q#44796, answer score: 5
Revisions (0)
No revisions yet.