5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Am trying to make sense of what you've written. It might be helpful if you could give a sample answer. \$\endgroup\$ – Siobhan Jun 27 '16 at 14:19
  • \$\begingroup\$ How do you intend to handle parsing failures? Does a simple boolean provide sufficient information for the error handling code? \$\endgroup\$ – Pieter Witvoet Jun 27 '16 at 15:11
  • \$\begingroup\$ @PieterWitvoet In this case a Boolen is sufficient as I will just say the expression sintax is incorrect. \$\endgroup\$ – Miguel Moura Jun 27 '16 at 15:12
13
\$\begingroup\$

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.
}
\$\endgroup\$
  • \$\begingroup\$ I though HashSet was ordered that is why I used it ... I was using List but I removed it because it makes no sense to order twice by the same property, does it? Is there an OrderedHashSet? \$\endgroup\$ – Miguel Moura Jun 27 '16 at 15:02
  • \$\begingroup\$ There is not. But I think that a order expression occurring twice is a logical error. Should the result be sorted by the first or last occurrence of it? You could use a private hashset in addition to the public list, in order to test for uniqueness. \$\endgroup\$ – Olivier Jacot-Descombes Jun 27 '16 at 15:26
4
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Are the TryParse methods in the framework implemented in this cumbersome way? I don't think so \$\endgroup\$ – edc65 Jun 27 '16 at 21:35
  • \$\begingroup\$ After a fast search in the source code: yes, sometimes. For instance there is a DateTimeParse class. For numbers, the parsing is in the Number class \$\endgroup\$ – edc65 Jun 27 '16 at 21:41
  • \$\begingroup\$ @edc65 have you heard about the Single responsibility principle? More often then not it is a good habit to try to keep it that way ;-) \$\endgroup\$ – t3chb0t Jun 28 '16 at 4:04
1
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Using a static method is pretty natural to me. Creating an instance first and then try to parse something into it is quite an untipattern. \$\endgroup\$ – t3chb0t Jun 27 '16 at 15:35
  • \$\begingroup\$ @t3chb0t, I disagree. Can you elaborate? Because I can think of multiple scenarios, where this can be useful. For example, imagine injecting this class into multiple places. And then you want to update it. With static TryParse method you gonna have to do the injecting all over again. With non-static method you call TryParse once and that's it, all the references are updated automatically. This is a really common pattern, and I don't see anything "anti" about it, if the situation calls for it. \$\endgroup\$ – Nikita B Jun 28 '16 at 7:43
  • 1
    \$\begingroup\$ Then I'd call it Update for what it really does. TryParse should only try to create a new object. I'd be really supprised if I found that it updates all references instead of just returning a new instance of something. \$\endgroup\$ – t3chb0t Jun 28 '16 at 8:00
  • \$\begingroup\$ Yes, I agree that Update or, say, Load might be better names in that case. \$\endgroup\$ – Nikita B Jun 28 '16 at 9:48
  • \$\begingroup\$ Ah, and as for surprise factor - I don't see it. It makes perfect sense to me that an instance method (whatever the name) changes the internal state of given instance. That's how 99% of classes work (yes, String, I'm looking at you). All it takes is one look at the signature of the method to eliminate any possible ambiguity. \$\endgroup\$ – Nikita B Jun 28 '16 at 12:05
1
\$\begingroup\$

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; }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.