Validation that also returns error messages

Question

My Create, Update and Delete methods should return a success and errors list depending up on the result. I want to avoid the out parameters in the method. Therefore I came up with an implementation of ResultModel class which gets returned for the above mentioned methods.

Is my implementation elegant enough to go ahead with the real world project? Or could there be other better ways to return the result?

Here is the sample code which actually works. I have inserted comments in the code where I think is necessary.

public sealed class CountryModel
{
    public string CountryName { get; set; } //mandatory and upto 32 chars
    public string CountryCode { get; set; } //mandatory and upto 5 chars
}

/// <summary>
/// To hold the results, be it the list of errors or just the success message.
/// </summary>
public sealed class ResultModel
{
    private object _success;        // to hold boolean and int value.
    public object Success { get { return _success; } }

    private List<string> _errors;   // to hold multiple errors, could be validation errors.
    public List<string> Errors { get { return _errors; } }

    public void AddSuccess(object any)
    {
        this._success = any;
    }

    public void AddError(string text)
    {
        if (string.IsNullOrEmpty(text))
        {
            return;
        }

        if (this._errors == null)
        {
            this._errors = new List<string>();
        }

        this._errors.Add(text);
    }
}

public sealed class CountryManager
{
    private const string EmptyErrorSuffix = "is empty.";
    private const string LengthErrorSuffix = "has exceeded it's length.";
    private const short CountryCodeMaxLength = 5;
    private const short CountryNameMaxLength = 32;

    public ResultModel AddCountry(CountryModel model)
    {
        var result = this.ValidateTheModel(model);
        if (result.Errors == null)   //if no errors then perform the Database operation, in this case Add a record.
        {
            try
            {
                result.AddSuccess(DatabaseMockClass.Add(model));
            }
            catch (Exception ex)
            {
                result.AddError(ex.Message); //add to the same model, if DB operation throws a unhandled exception.
                throw;
            }
        }

        return result;
    }

    private ResultModel ValidateTheModel(CountryModel model)
    {
        var _result = new ResultModel();

        if (model == null)
        {
            _result.AddError(string.Format("Country Model{0}", EmptyErrorSuffix));
        }
        else
        {
            _result.AddError(CheckString(model.CountryName, "Country Name", CountryNameMaxLength));
            _result.AddError(CheckString(model.CountryCode, "Country Code", CountryCodeMaxLength));
        }

        return _result;
    }

    private string CheckString(string text, string fieldName, short maxLength)
    {
        string errorText = string.Empty;

        if (string.IsNullOrEmpty(text))
        {
            errorText = string.Format("{0} {1}", fieldName, EmptyErrorSuffix);
        }
        else
        {
            if (text.Length > maxLength)
            {
                errorText = string.Format("{0} {1}", fieldName, LengthErrorSuffix);
            }
        }

        return errorText;
    }
}

internal static class DatabaseMockClass
{
    public static int Add(CountryModel model)
    {
        return 1; //Assume everything went well and return the Identity Value from the insert which is CountryId
    }

    public static bool Update(CountryModel model)
    {
        return true;
    }

    public static bool Update(int countryId)
    {
        return true;
    }
}

Here is the Program class to call the above code for testing. It also has some helper methods which are mentioned in the Main function.

    using System;

namespace TestConsole
{
    public class Program
    {
        static void Main(string[] args)
        {
            HelperAddMethod(string.Empty, "123456");   //Returns:
                                                       //Country Name is empty.
                                                       //Country Code has exceeded it's length.


            HelperAddMethod(string.Empty, "12345");    //Returns: Country Name is empty.

            HelperAddMethod("Anywhere in the world.", "12345");    //Returns: 1.
        }

        private static void HelperAddMethod(string countryName, string countryCode)
        {
            var model = new CountryModel()
            {
                CountryName = countryName,
                CountryCode = countryCode
            };

            var result = new CountryManager().AddCountry(model);
            if (result.Errors == null)
            {
                PrintSuccess(result);
            }
            else
            {
                PrintErrors(result);
            }
        }

        private static void PrintErrors(ResultModel model)
        {
            foreach (var item in model.Errors)
            {
                Console.WriteLine(item);
            }

            Console.ReadKey();
        }

        private static void PrintSuccess(ResultModel model)
        {
            Console.WriteLine("Success value: {0}.", model.Success);
            Console.ReadKey();
        }
    }
}

For validations, I will be using FluentValidation library. So, the validations I have written here is just a sample (I didn't want to complicate this question's code).

Answer 1

This is a more elegant solution as we have incorporated generics. I've started with 2 constructors, with a single parameter result indicating success. One could build off more functionality as needed. Instead of object, one can now specify the type of result model to return. I believe this makes the code a little more explicit. Sample code include implementation and sample usage below based on returning a 'Foo' object.

The boolean flag success (Success) indicates overall status. The result code (ResultCode) is a number which can be used to communicate additional status. On success this would be zero. On failure conditions this could be a number indicating a certain condition occurred. This could be business or technical errors. For example, 10 = Not Authorized, 20 = No records Found, 99 = Unexpected Error and so forth. The reason I like to include a result code is for client/server or service type situations. We would just send the number back to the client. The client would then do the proper localization and prepare the appropriate string based on the number. If one doesn't have to worry about that, just the error string can be returned and the result code can be omitted.

 public class ResultModel<T>
    {
        public ResultModel(bool success, int resultCode, T result)
        {
            Success = success;
            ResultCode = resultCode;
            Result = result;
            Errors = new List<string>();
        }

        public ResultModel(T result)
        {
            Success = true;
            ResultCode = 0;
            Result = result;
            Errors = new List<string>();
        }

        public bool Success { get; private set; }
        public int ResultCode { get; private set; }
        public T Result { get; private set; }
        public IList<string> Errors { get; private set; }

        public void AddError(string error)
        {
            Errors.Add(error);
        }
    }


public class Foo
    {
        public string Name { get; set; }
        public bool Flag { get; set; }
    }



public class FooResult
    {
        public ResultModel<Foo> GetHappyFooResult()
        {
            var foo = new Foo {Name = "Test", Flag = false};
            return new ResultModel<Foo>(foo);
        }

        public ResultModel<Foo> GetSadFooResult()
        {
            var foo = new Foo {Name = "Test", Flag = false};
            var result = new ResultModel<Foo>(false, 999, foo);
            result.AddError("Something bad happened.");
            return result;
        }
    }

Answer 2

I'm not sure I really buy into this as an idea. I can see it being useful where you want to do things like collect validation errors, but I'm not sure I'd want it to be responsible for swallowing exceptions and putting them into a list. Also, in your AddCountry method you're doing this:

catch (Exception ex)
{
    result.AddError(ex.Message); //add to the same model, if DB operation throws a unhandled exception.
    throw;
}

Since you're rethrowing and result is a local variable, it's going to be lost. I'm guessing you probably shouldn't be rethrowing and should be falling through to return result.

Answer 3

The first obvious problem in the code isn't really what you wanted reviewed, but

    public string CountryName { get; set; } //mandatory and upto 32 chars

is going to run into trouble with the Former Yugoslav Republic of Macedonia.

---

As for ResultModel, the purist OO way of doing it would be to have subclasses SuccessResultModel and ErrorResultModel. But if you're not going to take the purist approach, you should at least have a clear way to distinguish the two cases. At present it seems that I have to choose whether to test Success == null or Errors == null || !Errors.Any().

Actually, it's worse than that, because it's possible that the instance isn't yet initialised and has neither success nor error. Perhaps the builder pattern would be worth employing.

---

One other thing:

    public List<string> Errors { get { return _errors; } }

is only suitable if you want to allow me to fetch Errors and then modify its contents. Otherwise you should either use AsReadOnly() or declare the return type to be IEnumerable<string>.

Answer 4

Very suspicious code with DatabaseMockClassbecause it is hardcoded into CountryManager.AddCountry method.
How do you deal with it ?! Do you use this code locally and change this piece of code when it goes to test\real servers ? If yes it have to be remade. Look at dependency inversion principle

Answer 5

A tuple would be the most straightforward way of doing this sort of thing.

https://msdn.microsoft.com/en-us/library/system.tuple(v=vs.110).aspx

In your case just creating a subclass of the form:

public class Result:Tuple<T1,T2>{
   public Result(T1 val1,T2 val2):base(val1,val2){}
}

Would cut down on some boilerplate and give you an easy place to document what the tuple fields actually represent. However, you could just use Tuple directly.