Class with TryParse static method

Question

I am receiving and Expression (String) as follows:

String expression = "+name;-age;-city";

I parse this expression by splitting it using ";" and interpreting the +/- signs and create a List<OrderRule>. To wrap this I created the following:

public class OrderExpression {

  public HashSet<OrderRule> Rules { get; set; }

  public static Boolean TryParse(String expression, out OrderExpression orderExpression) {
    // Parse expression into a List<OrderRule>
    if (_parser.ExpressionIsValid(expression)) {
       orderExpression = new OrderExpression { Rules = _parser.Parse(expression) }
       return true;
    else
       return false; 
  }
}

So I would use it as follows:

String expression = "+name;-age;-city";
OrderExpression orderExpression;
OrderExpression.TryParse(expression, out orderExpression);

Does this make sense? I am not sure if this architecture and naming is the way to go.

I am being picky about this because I will use it as a standard for an API to convert an order expression into a List.

Answer 1

The TryParse or TryGetValue pattern is often used in the .NET Class Library and is a good approach. In order to fully comply with it, you should always return the type's default value default(T) for the output parameter when the parsing fails. For reference types this is always null:

public static bool TryParse(string expression, out OrderExpression orderExpression) {
    if (_parser.ExpressionIsValid(expression)) {
        orderExpression = new OrderExpression { Rules = _parser.Parse(expression) };
        return true;
    } else {
        orderExpression = null;
        return false;
    }
}

An out parameter must be assigned before leaving the method anyway. You'll get a compiler error otherwise.

---

Also, in C#, the C# type aliases are usually preferred over the .NET type names. I.e.: use string instead of System.String, bool instead of System.Boolean, int instead of System.Int32 and so on. (Personally, I prefer to use the .NET type name when accessing static members: Int32.TryPare(...).)

---

When calling a method, the return value can be ignored in C#, but here, it seems logical to react differently, depending on the outcome of the parsing:

if(OrderExpression.TryParse(expression, out orderExpression)) {
    // Use orderExpression.
} else {
    // Handle the error case.
}

---

A comment regarding the logic: Order expressions usually have to be executed in a specific order, however, a HashSet<T> is unordered. Return a List<OrderExpression> instead. Also, the setter can be private.

public List<OrderRule> Rules { get; private set; }

---

UPDATE

Since C# 6.0 you can declare getter-only auto-properties. You can set them in the constructor or in a property initializer.

To set the Rules property I declare a constructor. It can be private to force a client to use the static TryParse factory method.

public class OrderExpression
{
    private OrderExpression(List<OrderRule> rules)
    {
        Rules = rules;
    }

    public List<OrderRule> Rules { get; }

    public static bool TryParse(string expression, out OrderExpression orderExpression)
    {
        if (_parser.ExpressionIsValid(expression)) {
            orderExpression = new OrderExpression(_parser.Parse(expression));
            return true;
        } else {
            orderExpression = null;
            return false;
        }
    }
}

Since C# 7.0 you can declare the out-parameter in-line

string expression = "+name;-age;-city";
if (OrderExpression.TryParse(expression, out var orderExpression)) {
    //TODO: use orderExpression.
} else {
    //TODO: handle pare error.
}

Answer 2

If the list of rules should be immutable after parsing, consider to remove the setter of the Rules property and use ReadOnlyCollection instead of the HashSet:

public class OrderExpression
{
    private (IEnumerable<OrderRule> rules) {
        _rules.AddRange((rules ?? Enumerable.Empty<OrderRule>());
    }
    private readonly List<OrderRule> _rules = new List<OrderRule>();        
    public ReadOnlyCollection<OrderRule> Rules { get { return _rules.AsReadOnly(); } }

    public static Boolean TryParse(string expression, out OrderExpression orderExpression) {
        // Parse expression into a List<OrderRule>
        if (_parser.ExpressionIsValid(expression)) {
            orderExpression = new OrderExpression(_parser.Parse(expression))
            return true;
        }
        else {
            orderExpression = default(OrderExpression);
            return false;
        }
    }
}

Answer 3

Parsing the expression shouldn't a part of the OrderExpression. You should create another class that does nothing else but parsing. You might want to add some options to it later or unit-test it so it's better if the OrderExpression doesn't have to care about things that are not order-expression related (like the potential parser options etc.)

public class OrderExpressionParser
{
    public OrderExpression ParseOrderExpression(string value)
    {
        // parse the order expression....       
        return new OrderExpression(...);
    }
}

This way you can test the parser independantly and exchange it later without modifying the OrderExpression class that can remain as simple as:

public class OrderExpression
{
    public OrderExpression(List<OrderRule> orderRules) { ... }

    public IReadOnlyList<OrderRule> OrderRules { get; }
}

Then you would write:

var orderExpressionParser = new OrderExpressionParser();
var orderExpression = orderExpressionParser.ParseOrderExpression("+foo;-baz");

if (orderExpression == null)
{
    return;
}

 // do something...

---

If you however prefer the TryParse approach I'd add the parser as a static property to the OrderExpression:

public class OrderExpression
{
    public static OrderExpressionParser OrderExpressionParser { get; set; } 
        = new OrderExpressionParser();

    public OrderExpression(List<OrderRule> orderRules) { ... }

    public IReadOnlyList<OrderRule> OrderRules { get; }

    public static bool TryParse(string value, out OrderExpression result) =>
        (result = OrderExpressionParser.ParseOrderExpression(value)) != null;

Answer 4

IMHO, static TryParse method only makes sense for value types, when its impossible to mutate an existing instance and/or its hard to tell the difference between default(T) and successfully parsed value. For all other situations a constructor (for immutable types) or an instance method (for mutable types) will do a better job at keeping your code clean.

I am OK with this (for immutable objects):

// you can throw exception if string should be valid, but it is not
var expression = new OrderExpression(str);

Or this (for mutable objects which need to be updated):

var expression = new OrderExpression();
expression.TryParse(str);
//or 
//expression.TryUpdate(str);

Or this (good approach in general):

//for invalid strings: return null or throw
OrderExpression expression = _parser.Parse(str);

But using static method in your case does not feel right to me.

Answer 5

TellDontAsk principal sais:

Tell-Don't-Ask is a principle that helps people remember that object-orientation is about bundling data with the functions that operate on that data. It reminds us that rather than asking an object for data and acting on that data, we should instead tell an object what to do. This encourages to move behavior into an object to go with the data.

Will IComparer<T> implementation work for you instead of exposing rules collection? Let’s say we have:

public class Person
{
    public static readonly SortingParser<Person> Sorting = 
        new SortingParser<Person>()
            .OrderBy(p => p.Age)  
            .OrderBy(p => p.City, StringComparer.InvariantCultureIgnoreCase)
            .OrderBy(p => p.Name);                

    public string Name { get; set; }
    public int Age { get; set; }
    public string City { get; set; }
}

Then we can get IComparer<T> this way:

IComparer<Person> comparer;
Person.Sorting.TryParse("+Name;+Age;-City", out comparer);

Where:

public class SortingParser<T>
{
    public SortingParser()
        : this(ImmutableDictionary<string, IComparer<T>>.Empty)
    {
    } 

    SortingParser(ImmutableDictionary<string, IComparer<T>> comparers)
    {
        Comparers = comparers;
    }

    public SortingParser<T> OrderBy<V>(
        Expression<Func<T, V>> selector) =>
        OrderBy(selector, Comparer<V>.Default);

    public SortingParser<T> OrderBy<V>(
        Expression<Func<T, V>> selector, IComparer<V> comparer) =>        
        new SortingParser<T>(
            Comparers.Add(
                ((MemberExpression)selector.Body).Member.Name.ToLower(), 
                new RelayComparer<T,V>(selector.Compile(), comparer)));

    public bool TryParse(string expression, out IComparer<T> comparer)
    {
        comparer = CompositeComparer<T>.Empty;
        if (expression == null)
            return false;

        IComparer<T> ruleComparer;
        foreach (var rule in expression.Split(';'))
        {
            if (!Comparers.TryGetValue(
                rule.TrimStart('+', '-').Trim().ToLower(), 
                out ruleComparer))
            {
                comparer = null;
                return false;
            }

            switch (rule[0])
            {
                case '+':
                    comparer = comparer.ThenBy(ruleComparer);
                    break;

                case '-':
                    comparer = comparer.ThenByDescending(ruleComparer);
                    break;

                default:
                    comparer = null;
                    return false;
            }
        }

        return true;
    }

    ImmutableDictionary<string, IComparer<T>> Comparers { get; }
}

I use System.Collections.Immutable NuGet package and these helper library classes:

public static class ComparerComposition
{
    public static IComparer<T> Invert<T>(this IComparer<T> comparer) =>
        new InvertedComparer<T>(comparer);

    public static IComparer<T> ThenBy<T>(
        this IComparer<T> first, IComparer<T> second) =>
        new CompositeComparer<T>(first, second);

    public static IComparer<T> ThenByDescending<T>(
        this IComparer<T> first, IComparer<T> second) =>
        new CompositeComparer<T>(first, second.Invert());
}

And

public class InvertedComparer<T> : IComparer<T>
{
    public InvertedComparer(IComparer<T> inner)
    {
        Inner = inner;
    }

    public int Compare(T x, T y) =>
        Inner.Compare(y, x);

    IComparer<T> Inner { get; }
}

And:

public class CompositeComparer<T> : IComparer<T>
{
    public static readonly IComparer<T> Empty = new CompositeComparer<T>();

    public CompositeComparer(params IComparer<T>[] comparers)
    {
        Comparers = comparers;
    }

    public int Compare(T x, T y) =>
        Comparers
            .Select(c => c.Compare(x, y))
            .FirstOrDefault(r => r != 0);

    IComparer<T>[] Comparers { get; }
}

And:

public class RelayComparer<T, V> : IComparer<T>
{
    public RelayComparer(Func<T,V> selector, IComparer<V> parent)
    {
        Selector = selector;
        Parent = parent;
    }

    public int Compare(T x, T y) =>
        Parent.Compare(Selector(x), Selector(y));

    Func<T, V> Selector { get; }
    IComparer<V> Parent { get; }
}