Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Option 1 is surprising.

Option 1 is surprising.

I find it funny that you have return Enum.Parse(_propertyType, _parameter); in a parameterless Parse() method. Enum.Parse is just an example (int.Parse would be another); these methods are commonly used by C# programmers and your single-use parser seems to break POLS in my opinion.

###Option 2 is [more] consistent with the framework.

Option 2 is [more] consistent with the framework.

I don't mean to repeat what I just said, but it feels natural for a Parse method to take in all the parameters it needs. Now there is another problem here: your API is exposing object, which means the parsed value will need to be cast from the calling code, into the correct type. This is bad. Very bad. Nothing should ever be returning an object. If you're parsing a value type, you're boxing it right there. Is that not likely?

How about keeping it type-safe and taking a generic type argument instead of a Type parameter?

public T Parse<out T>(string value)
{
 ...
 return Convert.ChangeType(value, typeof(T));
}

This way if T is a value type, you're not boxing it. And you're not returning an object, and it's still the calling code that decides what type to parse the string with.

###Option 3 feels wrong.

Option 3 feels wrong.

Whenever I feel the need for a class to be called xxxxxHelper, something itches. The class should be static as well, if it's only going to expose static methods. That said, as @Simon has noted (oh, well, Simon says...) this brings DI to its knees. Best stay clear from that.

###Option 1 is surprising.

I find it funny that you have return Enum.Parse(_propertyType, _parameter); in a parameterless Parse() method. Enum.Parse is just an example (int.Parse would be another); these methods are commonly used by C# programmers and your single-use parser seems to break POLS in my opinion.

###Option 2 is [more] consistent with the framework.

I don't mean to repeat what I just said, but it feels natural for a Parse method to take in all the parameters it needs. Now there is another problem here: your API is exposing object, which means the parsed value will need to be cast from the calling code, into the correct type. This is bad. Very bad. Nothing should ever be returning an object. If you're parsing a value type, you're boxing it right there. Is that not likely?

How about keeping it type-safe and taking a generic type argument instead of a Type parameter?

public T Parse<out T>(string value)
{
 ...
 return Convert.ChangeType(value, typeof(T));
}

This way if T is a value type, you're not boxing it. And you're not returning an object, and it's still the calling code that decides what type to parse the string with.

###Option 3 feels wrong.

Whenever I feel the need for a class to be called xxxxxHelper, something itches. The class should be static as well, if it's only going to expose static methods. That said, as @Simon has noted (oh, well, Simon says...) this brings DI to its knees. Best stay clear from that.

Option 1 is surprising.

I find it funny that you have return Enum.Parse(_propertyType, _parameter); in a parameterless Parse() method. Enum.Parse is just an example (int.Parse would be another); these methods are commonly used by C# programmers and your single-use parser seems to break POLS in my opinion.

Option 2 is [more] consistent with the framework.

I don't mean to repeat what I just said, but it feels natural for a Parse method to take in all the parameters it needs. Now there is another problem here: your API is exposing object, which means the parsed value will need to be cast from the calling code, into the correct type. This is bad. Very bad. Nothing should ever be returning an object. If you're parsing a value type, you're boxing it right there. Is that not likely?

How about keeping it type-safe and taking a generic type argument instead of a Type parameter?

public T Parse<out T>(string value)
{
 ...
 return Convert.ChangeType(value, typeof(T));
}

This way if T is a value type, you're not boxing it. And you're not returning an object, and it's still the calling code that decides what type to parse the string with.

Option 3 feels wrong.

Whenever I feel the need for a class to be called xxxxxHelper, something itches. The class should be static as well, if it's only going to expose static methods. That said, as @Simon has noted (oh, well, Simon says...) this brings DI to its knees. Best stay clear from that.

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

###Option 1 is surprising.

I find it funny that you have return Enum.Parse(_propertyType, _parameter); in a parameterless Parse() method. Enum.Parse is just an example (int.Parse would be another); these methods are commonly used by C# programmers and your single-use parser seems to break POLS in my opinion.

###Option 2 is [more] consistent with the framework.

I don't mean to repeat what I just said, but it feels natural for a Parse method to take in all the parameters it needs. Now there is another problem here: your API is exposing object, which means the parsed value will need to be cast from the calling code, into the correct type. This is bad. Very bad. Nothing should ever be returning an object. If you're parsing a value type, you're boxing it right there. Is that not likely?

How about keeping it type-safe and taking a generic type argument instead of a Type parameter?

public T Parse<out T>(string value)
{
 ...
 return Convert.ChangeType(value, typeof(T));
}

This way if T is a value type, you're not boxing it. And you're not returning an object, and it's still the calling code that decides what type to parse the string with.

###Option 3 feels wrong.

Whenever I feel the need for a class to be called xxxxxHelper, something itches. The class should be static as well, if it's only going to expose static methods. That said, as @Simon has noted (oh, well, Simon says...) this brings DI to its knees. Best stay clear from that.

lang-cs

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