I'm creating an extension method that can be used on any type. The idea is that if this method is called, it checks if value is null. If null, it needs to return a default instance of the specified type.
public static dynamic CreateDefaultIfNull<T>(this object item)
{
if(item == null)
{
var type = typeof(T);
if(type.IsArray)
{
return Array.CreateInstance(type.GetElementType(), 0);
}
if(type.GetConstructor(Type.EmptyTypes) == null)
{
var paramCount = type.GetConstructors().First().GetParameters().Count();
var paramNullList = new List<object>();
for(int i = 0; i < paramCount; i++)
{
paramNullList.Add(null);
}
return Activator.CreateInstance(type, paramNullList.ToArray());
}
return Activator.CreateInstance(type);
}
return (T)item;
}
I'm looking for a review of my code, and any pitfalls that I may encounter or may have missed.
The idea is that this method should be used anytime that a null value cannot be used and a default must be returned.
I'm not worried about null values being passed into parameterized constructors, as this is doing. The constructor of those objects should be handling if null values are passed into their constructors.
EDIT: Based on the information provided, I've updated my code. The current version is below:
public static dynamic CreateDefaultIfNull<T>(this T item)
{
if (item != null)
return item;
var type = typeof(T);
if (type.IsArray)
{
return Array.CreateInstance(type.GetElementType(), 0);
}
if (type.GetConstructor(Type.EmptyTypes) == null)
{
var paramCount = type.GetConstructors().Min(construct => construct.GetParameters().Count());
var constructorToUse = type.GetConstructors().Where(construct => construct.GetParameters().Count() == paramCount).First();
var paramNullList = new object[paramCount];
var parameters = constructorToUse.GetParameters();
for (int i = 0; i < paramCount; i++)
{
paramNullList[i] = parameters[i].ParameterType.IsValueType ? Activator.CreateInstance(parameters[i].ParameterType) : null;
}
return Activator.CreateInstance(type, paramNullList);
}
return Activator.CreateInstance(type);
}
2 Answers 2
Took a quick look at your code and a few things came to mind.
As Magus mentioned in the comments it seems odd that you have a generic extension method on the object
class, which returns a dynamic type. You should (with a few modifications) be able to use the generic type instead:
public static T CreateDefaultIfNull<T>(this T item)
Since it rarely makes sense to check value types for null, you could also restrict it to reference types only.
public static T CreateDefaultIfNull<T>(this T item) where T : class
You will have to cast the result of CreateInstance
before returning.
The name is bothering me. The default value for a reference type is null, so if I saw your method used somewhere I would wonder what exactly it returns. (I don't know of a better name though. This method could return all kinds of things.)
I would invert the first if
statement. It would reduce some of the indentation and I think it looks cleaner:
if(item != null)
{
return item;
}
...
I haven't worked much with the GetConstructors
method, but in what order is the constructors returned? If there is no guarantee, can you be sure that the first constructor is the most appropriate?
I have often seen constructors that throw exceptions when given null. Is that the behavior you want?
EDIT: If you look here, you will see that there is no guarantee of the order and it is not recommended to depend on the order.
The GetConstructors method does not return constructors in a particular order, such as declaration order. Your code must not depend on the order in which constructors are returned, because that order varies.
Also, as mentioned in the comments, you should probably look into the situation mentioned by Jesse in the comments.
What will happen if the "first" constructor's parameter list has value types (such as int)..
It might/might not work as you expect in all situations. (I don't know what the defined behavior is)
You should be able to change
var paramNullList = new List<object>();
for(int i = 0; i < paramCount; i++)
{
paramNullList.Add(null);
}
return Activator.CreateInstance(type, paramNullList.ToArray());
to
var nullParams = new object[paramCount]; //All elements null by default.
return Activator.CreateInstance(type, nullParams);
Finally, your method doesn't work if there is no public constructor. What should happen in that case? You can't select the first constructor if there are none, so First
would throw an exception (InvalidOperationException
if I remember correctly).
-
\$\begingroup\$ When I change the return type from dynamic to T, the CreateInstance methods throw an error saying it can't cast to T. I've looked into the issue with the value types, attempting multiple value types as params, and it worked as expecting, giving the default value for the type as if I had called default(T). \$\endgroup\$Loren Shaw– Loren Shaw2014年05月06日 18:19:13 +00:00Commented May 6, 2014 at 18:19
-
\$\begingroup\$ With the issue on GetConstructors, allowing nulls passed in is acceptable in this case. If the construction requires values that aren't null, then I want it to throw the exception in that object with the NullPointerException. This helps ensure that if this occurs, that the nulls get handled in the constructor, or that they are explicitly creating a new object with the proper parameters. \$\endgroup\$Loren Shaw– Loren Shaw2014年05月06日 18:25:39 +00:00Commented May 6, 2014 at 18:25
-
\$\begingroup\$ Seems odd you can't cast to
T
. Create instance is supposed to create an instance of typeT
. If it is the Array.CreateInstance, try to cast to anobject
before casting toT
. \$\endgroup\$MAV– MAV2014年05月06日 18:27:43 +00:00Commented May 6, 2014 at 18:27 -
1
-
1\$\begingroup\$ If it is the expected behavior I will recommend that you throw the exception yourself and not rely on the
First
method to throw the exception. That way it is clear that it really is intended behavior and you can provide an appropriate message. \$\endgroup\$MAV– MAV2014年05月06日 19:48:45 +00:00Commented May 6, 2014 at 19:48
The biggest problem for me is that this is an extension method on System.Object
and returns a dynamic
.
Because this is already generic on T
, T
can be inferred if used as the parameter and return type. It will make invocation simpler at the very least.
int
)? Will assigningnull
to it blow up? \$\endgroup\$object
and returnsdynamic
. You couldn't just useT
? Then you'd get type inference at the call site, which might be nice. \$\endgroup\$