Skip to main content
Code Review

Return to Revisions

2 of 2
Commonmark migration

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.

Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467
default

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