Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Took a quick look at your code and a few things came to mind.


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

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

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

Added documentation regarding the order of constructors returned from GetConstructors
Source Link
MAV
  • 301
  • 2
  • 7

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

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?


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

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

Source Link
MAV
  • 301
  • 2
  • 7

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?


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

lang-cs

AltStyle によって変換されたページ (->オリジナル) /