Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
    }
share|improve this question
2  
What will happen if the "first" constructor's parameter list has value types (such as int)? Will assigning null to it blow up? –  Jesse C. Slicer May 6 at 14:57
    
I attempted that, it assigned the default value for the value type. For example, int defaulted to 0, bool defaulted to false, etc. –  Daniel Shaw May 6 at 15:00
2  
Huh, cool. Not sure if that's defined behavior, but I like the behavior! –  Jesse C. Slicer May 6 at 15:08
    
Me too, I was afraid of having to make this more complicated for value types. –  Daniel Shaw May 6 at 15:09
4  
I'm slightly horrified that this is an extension on object and returns dynamic. You couldn't just use T? Then you'd get type inference at the call site, which might be nice. –  Magus May 6 at 15:46

2 Answers 2

up vote 5 down vote accepted

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

share|improve this answer
    
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). –  Daniel Shaw May 6 at 18:19
    
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. –  Daniel Shaw May 6 at 18:25
    
Seems odd you can't cast to T. Create instance is supposed to create an instance of type T. If it is the Array.CreateInstance, try to cast to an object before casting to T. –  MAV May 6 at 18:27
1  
I just looked it up and found this: The GetConstructors method does not return constructors in a particular order, such as declaration order. –  MAV May 6 at 19:11
1  
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. –  MAV May 6 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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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