One instance per method call or multiple method call per instance

By Posted on

Problem

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?”.

Solution

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.

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

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

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");

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.

Leave a Reply

Your email address will not be published. Required fields are marked *