11
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ exceeded it's length - should be its. Also in string.Format("Country Model{0}", EmptyErrorSuffix), you need a space e.g. Model {0}. \$\endgroup\$ – 404 Jul 26 '16 at 10:08
  • \$\begingroup\$ Thanks for correcting the grammatical mistake and spacing issue. Just to be clear I have written a sample code for this question, am not allowed to post production code. Thanks anyway. \$\endgroup\$ – Coder Absolute Jul 26 '16 at 10:11
  • 1
    \$\begingroup\$ Why are you using object instead of a generic type parameter? \$\endgroup\$ – Pieter Witvoet Jul 26 '16 at 13:21
  • \$\begingroup\$ @Pieter Witvoet So that I can store bool or int, but your question made me think... I can separate that out into two variables? \$\endgroup\$ – Coder Absolute Jul 26 '16 at 13:30
  • 2
    \$\begingroup\$ You may want to read up on generics. Take List<T> for example: you can create a List<string>, which only holds strings, or a List<int>, which only holds integers. Likewise, if you turn ResultModel into a generic class, you can create a ResultModel<bool> or a ResultModel<int>, and have their Success property be typed accordingly. This makes it obvious what your methods return, and prevents you from accidentally returning (or casting to) the wrong type. \$\endgroup\$ – Pieter Witvoet Jul 26 '16 at 13:58
7
\$\begingroup\$

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;
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Would mind add a small description as to why this is better than what OP had ? At Code Review we want to help people get better, not just provide better solution. \$\endgroup\$ – Marc-Andre Jul 26 '16 at 19:05
  • 1
    \$\begingroup\$ @Marc - Sorry first post, I've updated my answer. Appreciate the comments and feedback. \$\endgroup\$ – Jon Raynor Jul 26 '16 at 20:20
  • \$\begingroup\$ @Jon Raynor: Can you please explain what's the use of ResultCode? \$\endgroup\$ – Coder Absolute Jul 27 '16 at 3:50
  • \$\begingroup\$ @Coder, I've updated my answer with more details of the result code and it's purpose. Thx. \$\endgroup\$ – Jon Raynor Jul 27 '16 at 14:01
  • \$\begingroup\$ @JonRaynor: Am going to implement a solution around your answer. Maybe, I will post a follow up review (will add a comment here, if I do that). Many thanks for the help! \$\endgroup\$ – Coder Absolute Jul 27 '16 at 14:21
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for highlighting about re-throwing an exception. I agree with you! Would you implement rest of the concept, if you arise in this situation where the method is able to throw multiple validation errors? \$\endgroup\$ – Coder Absolute Jul 26 '16 at 10:31
  • 1
    \$\begingroup\$ you mention 'Single custom exception' that means you would be concatenating the validation message I assume? Thinking towards the AggregateException is a best practice for validation messages handling than implementing your own? I clearly see the advantage of using AggregateException class because it's built into .NET and can be used in multiple assemblies. \$\endgroup\$ – Coder Absolute Jul 26 '16 at 11:21
  • \$\begingroup\$ Am a bit confused now, a code will be very helpful. =) \$\endgroup\$ – Coder Absolute Jul 26 '16 at 11:34
  • \$\begingroup\$ @CoderAbsolute I've mocked something up that hopefully makes more sense. It's a bit of a contrived example, because each scenario wouldn't typically be encountered in such close proximity but hopefully it makes more sense than my ramblings in the comments: ideone.com/b9ixhj \$\endgroup\$ – forsvarir Jul 26 '16 at 14:41
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestions! How about I implement a new property HasErrors which returns the bool value something like this: public bool HasErrors { get { return (_errors == null) ? false : Convert.ToBoolean(_errors.Count); } } Then, it will be possible to check something like this: if (!result.HasErrors) then do something else do something else Does it make sense to you? \$\endgroup\$ – Coder Absolute Jul 26 '16 at 11:16
  • \$\begingroup\$ That would be one way of doing it, although I'd write the condition as _errors != null && _errors.Any() or _errors != null && _errors.Count > 0. The alternative would be to make an IsSuccessful property, which might give more readable code on the assumption that you expect actions to succeed most of the time. \$\endgroup\$ – Peter Taylor Jul 26 '16 at 11:46
  • \$\begingroup\$ I disagree with your use of subclasses. Subclasses should be used to share and extend behaviour. I don't see how SuccessResult and ErrorResult would differ in behaviour and would instead only be different in name, which would end up with you trying to check the type at runtime. There's something to be said for OP's ResultModel being convoluted, but I don't think inheritance is a good solution here. \$\endgroup\$ – Dan Pantry Jul 26 '16 at 21:21
  • \$\begingroup\$ @DanPantry, I concede that it would have to be combined with a visitor pattern or something similar. \$\endgroup\$ – Peter Taylor Jul 27 '16 at 11:01
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ You have a fair point, however I get the impression that the DBMock is literally just to act as a stub (and give use a way of running the code) whilst developing the ResultModel classes the OP is interested in getting reviewed. They would presumably then map this code onto their production code which actually has some kind of DB layer, which may or may not be the same. \$\endgroup\$ – forsvarir Jul 26 '16 at 14:49
  • \$\begingroup\$ @Disappointed: Yes, on the real production code I will not be using DatabaseMockClass but the Method Signature and input parameters would be the same with slightly more methods. I mimic the production code just for this question. Thanks for your concern. \$\endgroup\$ – Coder Absolute Jul 27 '16 at 1:34
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Tuples are a quick and easy solution, but the disadvantage is that their property names aren't very descriptive. If you're going to document them you might as well create your own class with clear, descriptive property names. \$\endgroup\$ – Pieter Witvoet Jul 26 '16 at 21:58
  • \$\begingroup\$ @pieterWitvoet That is what doc strings, and possibly other external design documents are for. Often I find that people's supposedly descriptive names don't capture things like business intent and the like. \$\endgroup\$ – Kevin Jul 26 '16 at 23:51
  • \$\begingroup\$ ...wat? Please explain. \$\endgroup\$ – Dan Pantry Jul 27 '16 at 6:34
  • 2
    \$\begingroup\$ @Kevin: I'm not arguing against documentation, I'm arguing for readable code. Descriptive names aren't always sufficient, but that's not an argument for sticking to clearly undescriptive names such as Item1 and Item2. \$\endgroup\$ – Pieter Witvoet Jul 27 '16 at 7:04
  • \$\begingroup\$ Agreed, @PIeter. If I have to refer to external documentation (or any documentation) to have even a clue of what your code is doing, I'm going to re-enact a scene from The Shining \$\endgroup\$ – Dan Pantry Jul 27 '16 at 8:27

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.