Problems with the ICritic interface

[guilhermeluizsp]

Analyzing the structure of the ICritic interface, I'd say that it represents an error. There's only one implementation of this class on the Framework, and it is the Critic class, which confirms my theory.

https://github.com/Avanade/Liquid-Application-Framework/blob/93f7874f2ead13d3602cc39fc1e9355206217f57/src/Liquid.Base/Interfaces/CriticHandler/ICritic.cs#L10-L31

https://github.com/Avanade/Liquid-Application-Framework/blob/93f7874f2ead13d3602cc39fc1e9355206217f57/src/Liquid.Domain/CriticHandler/Critic.cs#L9-L77

This interface has several methods, such as "AddError" or "AddWarning", but these methods don't add anything, they just change the state of the class, which makes me think that those names aren't very clear. They could be renamed to "SetAsError" or "SetAsWarning".

But let's not end it here.

An error should be immutable, right? You can't (or shouldn't) turn a warning into an info. Semantically speaking, they're different things. If you have a warning and want to replace it to an info, naturally you should remove the warning first (as it's not a warning anymore) and add a new info. It also works with messages. If you want to change the message of an error, you actually want to remove the old one and add a new error (as it points to a different thing).

It also violates the Open/Closed Principle, as if there's a need to add a new method (such as "AddRecomentation"), we would need to change the interface.

With this in mind, what's the point (or the benefit) of having an interface at all? The Critic class works on its own. There's absolutely no need for an interface to exist in this case.

Also, currently, the class doesn't protects its invariants. Is it correct to have a null message or an empty code?

Initial solution

As mentioned in #35, the word "critic" is wrong for our context, so I renamed it to Problem, as it represents a problematic result. This class becomes immutable with factory methods.

/// <summary>
/// The type of a problem
/// </summary>
public enum ProblemType
{
    /// <summary>
    /// The operation has failed.
    /// </summary>
    Error = 1,

    /// <summary>
    /// The operation has succeded with warnings.
    /// </summary>
    Warning = 2,

    /// <summary>
    /// The operation has succeded with additional information.
    /// </summary>
    Info = 3
};

/// <summary>
/// Represents a problematic result of an operation.
/// </summary>
public class Problem
{
    /// <summary>
    /// A short code that uniquelly identifies the error.
    /// </summary>
    public string Code { get; }

    /// <summary>
    /// A text describing what happened. This may be used as a display value as it's intended to be human-readable.
    /// </summary>
    public string Message { get; }

    /// <summary>
    /// The type of the problem (e.g Error, Warning).
    /// </summary>
    public ProblemType Type { get; }

    /// <summary>
    /// Creates a new instance of a <see cref="Problem"/>.
    /// </summary>
    /// <param name="code">A short code that uniquelly identifies the error.</param>
    /// <param name="message">A human-readable message describing what went wrong.</param>
    /// <param name="type">The type of the problem (e.g Error, Warning).</param>
    public Problem(string code, string message, ProblemType type)
    {
        if (string.IsNullOrWhiteSpace(code))
            throw new ArgumentException("The code must not be empty.", nameof(code));

        if (string.IsNullOrWhiteSpace(message))
            throw new ArgumentException("The message must not be empty.", nameof(message));

        Code = code;
        Message = message;
        Type = type;
    }

    /// <summary>
    /// Creates an instance of <see cref="Problem"/> as an error (<see cref="Type"/> as <see cref="ProblemType.Error"/>).
    /// </summary>
    /// <param name="code">A short code that uniquelly identifies the error.</param>
    /// <param name="message">A human-readable message describing what went wrong.</param>
    /// <returns></returns>
    public static Problem AsError(string code, string message) => new Problem(code, message, ProblemType.Error);

    /// <summary>
    /// Creates an instance of <see cref="Problem"/> as a warning (<see cref="Type"/> as <see cref="ProblemType.Warning"/>).
    /// </summary>
    /// <param name="code">A short code that uniquelly identifies the error.</param>
    /// <param name="message">A human-readable message describing what went wrong.</param>
    /// <returns></returns>
    public static Problem AsWarning(string code, string message) => new Problem(code, message, ProblemType.Warning);

    /// <summary>
    /// Creates an instance of <see cref="Problem"/> as an info (<see cref="Type"/> as <see cref="ProblemType.Info"/>).
    /// </summary>
    /// <param name="code">A short code that uniquelly identifies the error.</param>
    /// <param name="message">A human-readable message describing what went wrong.</param>
    /// <returns></returns>
    public static Problem AsInfo(string code, string message) => new Problem(code, message, ProblemType.Info);
}

Benefits

• Solves #35 as the name is now clearer
• Adheres to the Open/Closed principle as if we need to add a new type, for example, we don't need to touch the implementation of the Problem class anymore. Factory methods are backwards compatible and we can even completely move them to helper classes, leaving the core class unchanged forever.
• Applies a fundamental functional programming principle (immutability)

What to consider

• With this approach, deserializing an object that contains a reference to a Problem could be a little bit annoying, as it doesn't have a public parameterless constructor. This however can be mitigated using custom converters. For instance, Json.NET (which currently powers a lot of parts of the framework) has an abstract class called JsonConverter, which solves this issue.
• It is a breaking change and very hard to make a smooth transition without breaking existing code.

We can work through this.

[bruno-brant]

We should try to implement this as soon as possible. We need a strategy to minimize impact to existing customers.