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 have the following class designs, and I'm looking for the best practices, aligned to the OOP design principles/patterns.

The ParameterParser will be used inside a foreach block through 10 items approximately.

Any comments regarding the method body will also be appreciated, though the focus of the question is on the class design/usage.

  1. Use the constructor to take the parameter and parse (one instance per parse)

    public class ParameterParser
    {
        private readonly Type _propertyType;
        private readonly string _parameter;
    
        public ParameterParser(Type propertyType, string parameter)
        {
            _propertyType = propertyType;
            _parameter = parameter;
        }
    
        public object Parse()
        {
            if (typeof(IEnumerable).IsAssignableFrom(_propertyType)
             || typeof(IEnumerable<>).IsAssignableFrom(_propertyType))
                return _parameter.Split(new[] { ',', ';', '|' }, StringSplitOptions.RemoveEmptyEntries);
    
            if (_propertyType.IsEnum)
                return Enum.Parse(_propertyType, _parameter);
    
            return Convert.ChangeType(_parameter, _propertyType);
        }
    }
    
  2. Parameterless constructor, multiple parsing per instance

    public class ParameterParser2
    {
        public object Parse(Type propertyType, string parameter)
        {
            //same code
        }
    }
    
  3. A helper class, static method

    public class ParameterHelper
    {
        public static object Parse(Type propertyType, string parameter)
        {
            //same code
        }
    }
    

I think the first one is more likely to be the best, but I can't explain which exactly are the advantages over the others. The question is "Considering design principles, which one is the best and why? Is there any pattern which would fit the scenario?".

share|improve this question
add comment

7 Answers 7

up vote 10 down vote accepted

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.

share|improve this answer
4  
Simon says: Upvote this answer! –  Simon André Forsberg Dec 17 '13 at 16:49
    
Haha thanks for your answer, but I'll have to ask over it: 1) I couldn't get the funny part (do you mean that it is useless, since Convert.ChangeType should work aswell?), Also, I didn't know that POLS principle, although it's very subtle and implicit, it's very nice to know that it has a name and is documented! 2) the type is known only at runtime, so generics are not likely to be the best option, unless you tell me that reflection performs better over unboxing ;-) 3) Certainly, something itches. I fell the same. I'm close to believe that helpers are i-dont-know-where-to-put-this-yet class. –  natenho Dec 17 '13 at 16:53
    
@natenho Principle Of Least Surprise (/Astonishment) :) what I meant was that you're using a parameterized Parse method inside a parameterless Parse method, being parameterless should feel wrong when all Parse methods in the framework take in their parameters. 2) bah, generics was just an attempt at something :) - maybe dynamic can come to the rescue then? 3) 100% agree! –  Mat's Mug Dec 17 '13 at 17:02
    
@natenho wait a minute, the type is known only at runtime - then what do you pass for a propertyType argument? Say you've got Parse(typeof(int), "123"), would turn to Parse<int>("123")... that wouldn't work? –  Mat's Mug Dec 17 '13 at 17:06
1  
ok, I don't want to increase the scope of the question here, but basically the context is that the parser is used to read a dictionary and pass its values to a instance of any given class (object properties are mapped to keys and are get/set via reflection). So the parser will always be called with a propetyInfo.PropertyType parameter. –  natenho Dec 17 '13 at 17:37
add comment

The important thing is to ask yourself: How would you use it?

If you are only going to parse with the same parameters multiple times, then the first method would be preferable since you only have to send it the parameters once. However, if you're doing the same thing over and over again then you should ask yourself if you really need to do it over and over again or if just once would be enough?

Since the class are only using the _propertyType and _parameter in one method, I see no need to keep them stored in the instance of the class, and therefore I like method 2. The way I see it, this is the option that best fits Open/Closed Principle.

Option 3 has a slight negative side that static methods can't be overridden (at least not in Java), and so it's harder to provide different implementations for it. However, if you are sure that you need one and only one implementation and that there's no need to override the method anywhere, then option 3 is fine from my perspective. Be aware that this option defies Dependency Inversion principle, since you would always need to know the actual implementing class of the method (unless you write another class for determining which static method to call, but then why have the method static in the first place?)

share|improve this answer
1  
I'd upvote but I'm out of ammo :) –  Mat's Mug Dec 17 '13 at 16:11
    
Not sure why you mention Java, the question is about C#. But overriding static methods doesn't make much sense anyway. –  svick Dec 17 '13 at 17:51
    
@svick It's simply because I know Java a lot better than C#, and I know that even though the languages are similar there are differences too. –  Simon André Forsberg Dec 17 '13 at 17:52
add comment
if (_propertyType.IsAssignableFrom(typeof(IEnumerable))
 || _propertyType.IsAssignableFrom(typeof(IEnumerable<>)))

This is wrong.

  1. IsAssignableFrom() is kind of confusing method, it works the other way around, it should be typeof(IEnumerable).IsAssignableFrom(_propertyType). If you leave it the way it is, it will match object, but not for example List<int>.
  2. IEnumerable<T> inherits from IEnumerable, so the second condition is not necessary.
  3. The second condition wouldn't work correctly anyway. Neither List<int> nor List<> match that (after the reverse from #1).
share|improve this answer
    
Wow! Thank you very much! I hadn't noticed it. It's very confusing indeed. –  natenho Dec 17 '13 at 18:16
    
I've just checked and the original code had already been fixed. –  natenho Dec 17 '13 at 18:21
add comment

Well first decide what type of object you are going to design whether its statefull or stateless. What I understood from your class purpose is to parse a parameter. The way you have designed is state full which is absolutely unnecessary and might be not thread safe (depends on usage).

Parsing is generic thing and it has no necessity to hold any state. Fine how would you use your class?

//Parsing first parameter 
ParameterParser  parser1 = new ParameterParser(“xx”,”yy”);
Object  parameter1 = Parser1.parse();

If I want to parse another parameter , I should create new instance which is unnecessary since its not statefull object .

ParameterParser  parser2 = new ParameterParser(“AA”,”BB”);
Object  parameter2 = Parser2.parse();

Instead of this you could make it as stateless object and its thread safe.

//You can also think of make it singleton or static since its stateless 
ParameterParser  parser1 = new ParameterParser();
Object1 parameter1 =Parser1.parse(“xx”,”yy”);
Object1 parameter2 =Parser1.parse(“AA”,”BB”);

// You can also think of accessors method based implementation to change the behaviour at run-time. If you feel some other operations/methods are depends on your variables passed in then consider this one... This is not thread safe by nature but you can make it as thread safe.  
ParameterParser  parser1 = new ParameterParser();
parser1.setXXX("xxx");
share|improve this answer
    
It's almost the essence of the other answers: Statefull vs. Stateless and Static vs. Non-static implementation. +1 for raising the threading concern. I'm learning a lot with this question. –  natenho Dec 19 '13 at 14:09
add comment

I agree with the other posters in that it depends on how the class is to be used.

In addition, for completeness, I'd like to propose a fourth pattern.

This pattern is intended for a specific sort of use-- dealing with parameters as stateful objects that need to be worked with in a variety of ways. To do this well, the new design will have to address a couple issues:

  1. Stateful classes that have a name ending with "er" should set off a warning. If a class is stateful, it represents a thing, not an action; "parser" "controller" "helper" are all suspicious in this case. The class IS a thing, it doesn't DO a thing (the methods do). (Another warning sign is that the class has only one method. It indicates procedural rather than object-oriented thinking.)

  2. Type conversion, by convention in the CLR, is performed using methods that begin with "To" e.g. "ToString()" or "Convert.ToInt32"

  3. It would be nice if we could repeatedly call "Parse" without incurring overhead every time. In this example the overhead is very small but we're talking about general patterns here.

  4. The class ought to be extensible. For example, what if you need to compare parameters? It'd be nice to be able to implement IComparable. What if you want to be able to print it to logs? You'd want to override ToString. Etc.

So here's my solution:

public class Parameter : IComparable
{
    private readonly Type _type;
    private readonly string _value;
    private object _objectResult = null;

    public Parameter(Type type, string val)
    {
        _type = type;
        _value = val;

        //The following line is optional, depending if you want lazy evaluation vs. control over when the logic is executed
        //_objectResult = ParseAsObject();
    }

    private object ParseAsObject()
    {
        if (typeof(IEnumerable).IsAssignableFrom(_type)
             || typeof(IEnumerable<>).IsAssignableFrom(_type))
            return _value.Split(new[] { ',', ';', '|' }, StringSplitOptions.RemoveEmptyEntries);

        if (_type.IsEnum)
            return Enum.Parse(_type, _value);

        return Convert.ChangeType(_value, _type);
    }

    public object ToObject()
    {
        if (_resultObject == null) _resultObject = ParseAsObject();  //Not needed if you chose to uncomment the optional line above
        return _resultObject;
    }

    public override void ToString()
    {
        return ToObject().ToString();
    }

    public int CompareTo(object compareTo)
    {
        Parameter p = compareTo as Parameter;
        if (p == null) throw new ArgumentException("Can't compare to that!");
        return this.ToString().CompareTo(p.ToString());
    }
}

In general I tend to prefer the lazy evaluation approach (the values are only parsed when the parsed value is needed and not before) but again we are talking general patterns here. In some cases you will want to the caller to know exactly when the heavy lifting takes place, e.g. if the parsing operation requires a DB call or something expensive. I've given you the option with the commented-out lines above.

share|improve this answer
    
Very good approach, I really appreciated your issues list, specially the first item. But in this case, it's a class which needs to perform operations over another kind of object (every string value in a dictionary<string, string>, for instance). I don't think it should be stateful, but I'm thinking about the generality of your statement whether a class have to "DO a thing" or "BE a thing". Btw, although the class is extensible, YAGNI principle is a worth thinking, so I don't need to implement IComparable neither override ToString at this moment, then the class ends up with only one method. –  natenho Dec 18 '13 at 11:51
add comment

It seems using a generic class might be a good choice for this. See if this code fits what you're doing. Off the top of my head, I can't see where using Convert.ChangeType against a string is going to work except for the built-in primitive types or types that implicitly implement a string cast. But I guess if you're just parsing generic data though, that should be good enough.

The first method just shows implementations for a primitive, an enum, an array, and a generic IEnumerable.

I'm not clear about what you mean regarding it being used in a for loop. If your only using the function to recast your input, then a static method works fine here. Meaning that you don't have any other reason to store the property string for later use. If you do need to store it, then the static methods in the code below would need to be changed back to instance methods.

BTW. For the life of me I couldn't get the

if (typeof(IEnumerable).IsAssignableFrom(_propertyType) || typeof(IEnumerable<>).IsAssignableFrom(_propertyType))

line to work against arrays or generic lists so I changed that in my example as well.

Anyhow, I hope this helps. Good luck.

private void Test() {
    Console.WriteLine(ParameterParser<int>.Parse("2"));
    Console.WriteLine(ParameterParser<ButtonBorderStyle>.Parse("2"));
    Console.WriteLine(((string[])ParameterParser<int[]>.Parse("2, 3, 4, 5")).Length);
    Console.WriteLine(((string[])ParameterParser<int[]>.Parse("2|3|4|5")).Length);
    Console.WriteLine(((string[])ParameterParser<List<int>>.Parse("2, 3, 4, 5")).Length);
}

public class ParameterParser <T>{

    public static object Parse(string param) {

        try {
            if (typeof(T).IsArray || IsGenericList())
                return param.Split(new[] { ',', ';', '|' }, StringSplitOptions.RemoveEmptyEntries);

            if (typeof(T).IsEnum)
                return Enum.Parse(typeof(T), param);

            return Convert.ChangeType(param, typeof(T));

        } catch {
            return param;
            // throw new ArgumentException(param + "cannot be converted to " + typeof(T).ToString());
        }
    }
    private static bool IsGenericList(){
         foreach (Type type in typeof(T).GetInterfaces()) {   
            if (type.IsGenericType) {
                if (type.GetGenericTypeDefinition() == typeof(IEnumerable<>)) {
                    return true;
                }
            }
        }

        return false;
    }
}
share|improve this answer
    
Thanks for your answer, my actual code is currently very close to yours, but I can't use generics because T is only known during run-time, i.e. T is known via reflection. –  natenho Dec 17 '13 at 19:29
    
if you're already using reflection you could use Activator.CreateInstance dynamically at run-time. But either way, the IsGenericList should be useful. Again, good luck. –  drankin2112 Dec 17 '13 at 19:35
add comment

Although I'd like to know more about where it's being called from and where it's likely to be used in the future, my vote goes to #3.

  • #1 is over-complicating a simple task. The only reason it would make sense to take that approach is if you anticipate needing other parameter-related functions that also operate on propertyType and parameter, in which case the class would be Parameter, with Parse() as a function. In general if a class exists solely to hold one function, then there's either a better place for that function, or a more generic class that should be made instead.

  • #2 Likely better than #1 because it allows for a wider range of parameter-related functions that act on something other than propertyType and parameter, but the same criticism applies. Either name the class Parameter with an eye toward expanding it in the future, or go with #3.

  • #3 Completely acceptable and appropriate in this case. If this is your only parameter related helper function and you don't see a need for others in the future, then there is no reason to instantiate an object. This is a perfect case for a static function: takes a couple of parameters, returns the result, is completely ephemeral and stores no state information.

Overall, I generally favor the solution that does the minimum necessary and requires the least to use. Even if there are other parameter-related functions in the future, I would keep the class and functions static unless a need arose to save state information or take advantage of another instantiated-object feature.

share|improve this answer
    
code is only most simple when you cannot take anything from it. someone else said that before me but it fits. –  Malachi Dec 17 '13 at 22:13
    
I understand your point of view, and I confess that is very tempting to write static functions in several scenarios when you came from structural programming. As I told before, generally this kind of function is placed in a helper because they don't seem to fit in an object, and its behavior could be so simple that it may be considered a freeloader code smell. I still accept the #2 as the best, not just because its more elegant, but because it seems safer to consider that an object is more likely to survive a future refactoring / new implementation... –  natenho Dec 18 '13 at 1:26
add comment

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.