Is there an alternative to the Notification pattern for multiple messages and success/failure?

Question

Is there an alternative to the Notification pattern for multiple messages and success/failure?

I have a class, OperationResult, that I use to return a Success boolean and a list of "error" messages. These messages are sometimes unexpected errors but more often ordinary cases that often happen. Sometimes we return single error messages but other times we return several. I'm hoping to find a better way.

This seems to be more or less the Notification pattern advocated by Fowler. The consumers then do something reasonable with the success state and the errors, most often displaying errors to the user but sometimes continuing on in the case of non-fatal errors.

I thus have lots of service methods (not web service methods) that look something like this:

private ThingRepository _repository;
public OperationResult Update(MyThing thing)
{
    var result = new OperationResult() { Success = true };

    if (thing.Id == null) {
        result.AddError("Could not find ID of the thing update.");
        return result;
    }

    OtherThing original = _repository.GetOtherThing(thing.Id);
    if (original == null) return result;

    if (AnyPropertyDiffers(thing, original))
    {
        result.Merge(UpdateThing(thing, original));
    }

    if (result.Success) result.Merge(UpdateThingChildren(thing));
    if (!result.HasChanges) return result;

    OperationResult recalcResult = _repository.Recalculate(thing);

    if (recalcResult.Success) return result;

    result.AddErrors(recalcResult.Errors);
    return result;
}
private OperationResult UpdateThing(MyThing ) {...}
private OperationResult UpdateThingChildren(MyThing) {...}
private bool AnyPropertyDiffers(MyThing, OtherThing) {...}

As you can imagine, UpdateThing, UpdateThingChildren, and ThingRepository.Recalculate all have similar OperationResult merging/manipulating code interleaved with their business logic.

Is there an alternative to so much code munging around my returned object? I'd like my code to just focus on the business logic without having to be so particular about manipulating an OperationResult.

I'm hoping to instead have code that looks something like the following, something that better expresses its business logic with less message-handling cruft:

public ??? Update(MyThing thing, ???)
{
    if (thing.Id == null) return ???;
    OtherThing original = _repository.GetOtherThing(thing.originalId);
    if (original == null) return ???;

    if (AnyPropertyDiffers(thing, original))
    {
        UpdateThing(thing, original));
    }

    UpdateThingChildren(thing);
    _repository.Recalculate(thing); 
    return ???;  
}

Any ideas?

Note: throwing exceptions isn't really appropriate here as the messages aren't exceptional.

Answer 1

I think this a situation where functional programming can help, so I'd try with a package porting some F# feautures to C#

using Optional;

and since we want to manage exceptions

using Optional.Unsafe;

At this point we can introduce a helper, to do the typical functional "monad chaining"

public static class Wrap<Tin, Tout>
{
    public static Option<Tout, Exception> Chain(Tin input, Func<Tin, Tout> f)
    {
        try
        {
            return Option.Some<Tout,Exception>(f(input));
        }
        catch (Exception exc)
        {
            return Option.None<Tout, Exception>(exc);
        }
    }
    public static Option<Tout, Exception> TryChain(Option<Tin, Exception> input, Func<Tin, Tout> f)
    {
        return input.Match(
                some: value => Chain(value, f),
                none: exc => Option.None<Tout, Exception>(exc)
            );
    }
}

Now, assuming we have the following updates, that can throw exceptions:

    Type2 Update1 (Type1 t)
    {
        var r = new Type2();
        // can throw exceptions
        return r;
    }
    Type3 Update2(Type2 t)
    {
        var r = new Type3();
        // can throw exceptions
        return r;
    }
    Type4 Update3(Type3 t)
    {
        var r = new Type4();
        // can throw exceptions
        return r;
    }

we'll be able to write a logical flow just following the Happy Path

    Option<Type4, Exception> HappyPath(Option<Type1, Exception> t1)
    {
        var t2 = Wrap<Type1,Type2>.TryChain(t1, Update1);
        var t3 = Wrap<Type2, Type3>.TryChain(t2, Update2);
        return Wrap<Type3, Type4>.TryChain(t3, Update3);
    }

Finally, with an extension class like

public static class Extensions {
    public static Option<Type2, Exception> TryChain(this Option<Type1, Exception> input, Func<Type1, Type2> f)
    {
        return Wrap<Type1, Type2>.TryChain(input, f);
    }
    public static Option<Type3, Exception> TryChain(this Option<Type2, Exception> input, Func<Type2, Type3> f)
    {
        return Wrap<Type2, Type3>.TryChain(input, f);
    }
    public static Option<Type4, Exception> TryChain(this Option<Type3, Exception> input, Func<Type3, Type4> f)
    {
        return Wrap<Type3, Type4>.TryChain(input, f);
    }
}

the Happy Path can be written in a beautiful form

    Option<Type4, Exception> HappyPath(Option<Type1, Exception> t1)
    {
        var t2 = t1.TryChain(Update1);
        var t3 = t2.TryChain(Update2);
        return t3.TryChain(Update3);
    }

Answer 2

I would argue that your service is not doing one thing. Its responsible for validating input and then if validation succeeds updating things. And yes I agree that user needs as many information about errors (violations, name not provided, description to long, date end before date start etc) as you can produce on single request and with that the exceptions are not the way to go.

In my projects I tend to separate concerns of validation and update so the service that does the update has little to none chance of failure. Also I like the strategy pattern to do both validation and update - user requests a change the generic service accepts request of validation/update, calls specific validator/updater which in turn calls generic service to validate/update some dependencies. The generic service will merge results and decide upon success or failure of the operation. The obvious benefit is that violation messages merging is done once in some generic class and specific validator/updater can focus on single entity. On the other hand you might want to validate some database uniqueness or existence of objects on the database this exposes two concerns: extra queries to database (light queries that use Exist to minimize output but its a trip to the database) and the latency between validation and update (in that time database can change and your uniqueness or existence verification can change (this time is relatively small but it can happen). This pattern also minimizes duplication of UpdateThingChildren - when you have simple many to many relation the child can be updated from either one of connected entities.

Answer 3

First, to shortly answer your question, there is no alternative to Notification pattern for combining multiple responses into one. Even if you could throw an exception, you'd have AggregateException which is nothing other than Notification pattern for collecting several exceptions into one (exception is just one kind of output that the method can have).

Notification pattern is a great pattern and I don't see a reason to avoid it actually. Yes, your service layer methods look somewhat chatty, indeed, but those could be refactored. While you have not really asked for the advice how this could be refactored, you need to think about that part mostly.

Couple of advises by looking at your code and in general:

It's normal to make Notification pattern a primary pattern in your codebase if applicable. Not just in the service layer, but everywhere else too. If a method returns more than a primitive value as a result, I don't see how else you would do it. So, every method could return OperationResult{TResult} (generic), which indicates the success/failure, as well as the result of the operation - list of errors if failure, TResult object if success. Every caller method will decide what to do with the result - either discard part or all of it, or return it to its caller up in the call stack.

In your code, you have UpdateThingChildren private method. I'm not sure what that does, but it would be much better expressing the intention if you do thing.UpdateChildren() call on the thing itself.

To decrease the chattiness of your service method, you could use fluent interface-like method chaining. It should not be difficult to implement, assuming that every operation you call returns OperationResult. I'd expect your code to look like this at a minimum:

private ThingRepository _repository;
public OperationResult Update(MyThing thing)
{
    return new OperationResult() //removed Success = true, just make that a default value.
        .Then(() => thing.ValidateId()) //moved id validation into thing
        .Then(() => GetOtherThing(thing.Id)) //GetOtherThing validates original is null or not
        .Then(() => thing.AnyPropertyDiffersFrom(original)) //moved AnyPropertyDiffers into thing
        .Then(() => thing.UpdateChildren())
        .Then(() => _repository.Recalculate(thing));
}
private OperationResult GetOtherThing(MyThing ) {...}

As you can imagine, implementing Then method is not difficult at all. It's defined on OperationResult and takes Func{OperationResult} (generic) as argument. It does not execute func if success == false. Otherwise, it executes func and merges the operation result with itself. At the end, it always returns itself (this).

Answer 4

Some people understand that patterns are not to be broken. But patterns have many critics. And there is really room for improvisation. You still can consider that you have implemented a pattern even if you modified some details. Pattern is general thing and can have specific implementation.

Saying that, you can choose not to go with such fine-grained operation result parsing. I always advocated for something like this pseudo-code

class OperationResult<T>
{
    List<T> ResultData {get; set;}
    string Error {get; set;}
    bool Success {get; set;}
}

class Consumer<T>
{
    void Load(id)
    {
        OperationResult<T> res = _repository.GetSomeData<T>(id);
        if (!res.Success)
        {
            MessageBox.Show(DecorateMsg(res.Error));
            return;
        }
    }
}

As you see, server code return data or error message. Simple. Server code does all the logging, you can write errors to DB, whatever. But I see no value passing complex operation results to consumer. Consumer only needs to know, success or not. Also, if you get multiple things in same operation, what the sense to continue operation if you fail get first thing? May be the problem that you trying to do too many things at once? It is arguable. You can do this

class OperationResult
{
    List<type1> ResultData1 {get; set;}
    List<type2> ResultData2 {get; set;}
    List<type3> ResultData3 {get; set;}
    string[] Error {get; set;} // string[3]
    bool Success {get; set;}
}

In this case you may fill 2 grids but not third one. And if any errors occur on client side because of this, you will need to handle it with client error handling.

You definitely should feel free to adjust any patterns for your specific needs.

Answer 5

I see two possible options that you could use while avoiding throwing exceptions, but in the end we are only reducing the amount of code needed for messages:

1. I would refactor the code a little bit in order to make it more standard across all calls. Like the following (See comments inside the parenthesis in the code comments):

   public OperationResult Update2(MyThing thing)
   {
       var original = _repository.GetOtherThing(thing.Id);
       if (original == null)
       {
           return OperationResult.FromError("Invalid or ID not found of the thing update.");
       }
   
       var result = new OperationResult() { Success = true };
       if (AnyPropertyDiffers(thing, original))
       {
           result.Merge(UpdateThing(thing, original));
           if (!result.HasChanges) return result;
       }
   
       result.Merge(UpdateThingChildren(thing));
       if (!result.HasChanges) return result;
   
       result.Merge(_repository.Recalculate(thing));
       return result;
   }
   

   • _repository.GetOtherThing - Delegate Id checking to the repository, for simpler code and to ensure we return an error if nothing happened
   • UpdateThing - With exit after no changes
   • _repository.Recalculate - We now merge results and return them

2. Use a scope class shared by all the services at the construction of the services.

   // We a scope class shared by all services, 
   // we don't need to create a result or decide what result to use. 
   // It is more whether it worked or didn't 
   public void UpdateWithScope(MyThing thing)
   { 
       var original = _repository.GetOtherThing(thing.Id);
       if (_workScope.HasErrors) return;
       if (original == null)
       {
           _workScope.AddError("Invalid or ID not found of the thing update.");
           return;
       }
   
       if (AnyPropertyDiffers(thing, original))
       {
           UpdateThing(thing, original);
           if (_workScope.HasErrors) return;
       }
   
       UpdateThingChildren(thing);
       if (_workScope.HasErrors) return;
   
       _repository.Recalculate(thing);
   }
   

   • _repository.GetOtherThing must add any errors to the _workScope
   • UpdateThing must add any errors to the _workScope
   • UpdateThingChildren must add any errors to the _workScope
   • _repository.Recalculate must add any errors to the _workScope

In this last example we don't need to return anything since the caller will have to verify if the scope is still valid. Now if there are multiple validations to be performed I would suggest doing another class like Rafal mentioned.

• Last note: I would throw exceptions for anything that should be handled in the GUI like the 'thing.Id' sent without a value or not sent at all, that sounds to me like whoever did the interface didn't handle this scenario properly or the current interface is old or unsupported, which normally is enough reason to throw an exception.

Answer 6

You can use exceptions internally without throwing them to callers. This allows you to break out of a failed operation easily, and groups your business logic all in one place. There is still some 'cruft', but it's separated (and could be in its own class) from the business logic, which is contained in the *Internal implementations. Not saying this is the best or only way of approaching this problem, but I may go with this:

public class OperationResult
{
    public bool Success { get; set; }
    public List<string> Errors { get; set;  } = new List<string>();
}
public class Thing { public string Id { get; set; } }
public class OperationException : Exception
{
    public OperationException(string error = null)
    {
        if (error != null)
            Errors.Add(error);
    }

    public List<string> Errors { get; set; } = new List<string>();
}

public class Operation
{
    public OperationResult Update(Thing thing)
    {
        var result = new OperationResult { Success = true };
        try
        {
            UpdateInternal(thing);
        }
        catch(OperationException e)
        {
            result.Success = false;
            result.Errors = e.Errors;
        }

        return result;
    }

    private void UpdateInternal(Thing thing)
    {
        if (thing.Id == null)
            throw new OperationException("Could not find ID of the thing update.");

        var original = _repository.GetOtherThing(thing.Id);
        if (original == null)
            return;

        if (AnyPropertyDiffers(thing, original))
            result.Merge(UpdateThing(thing, original));

        result.Merge(UpdateThingChildren(thing));

        if (result.HasChanges)
            _repository.Recalculate(thing);
    }
}

Answer 7

I would approach with State pattern and internal collection to save information. You can start applying events that change the state and store the information related to the applied event. Finally call get information to wrap them inside operationresult.

A pseudo code

public OperationResult Update(MyThing thing)
{
     return new OperationResult
            {
               Errors = thing.ApplyEvent(Event.NullCheck)
                             .ApplyEvent(Event.OriginalNullCheck)
                             .ApplyEvent(Event.OriginalPropertyDiffersCheck)
                             .CollectInfo(),
               Success = true
            };
}
public class MyThing
{
   private List<string> _errors = new List<string>();
   private MyThing _original;

   public MyThingState ThingState {get;set;}
   public MyThing ApplyEvent(Event eventInfo)
   {
       MyThingState.ApplyEvent(this, eventInfo)
   }        
}

public class NullState : MyThingState
{
    public MyThing ApplyEvent(MyThing myThing, Event eventInfo)
    { 
         if(mything == null)
         {
           // use null object pattern
           mything.AddErrors("Null value")
           // Based on the event, you select which state to instantiate
           // and inject dependencies
           mything.State = new OriginalNullState();
         }
    }
}
public class OriginalNullState : MyThingState
{
      public void ApplyEvent(MyThing myThing, Event eventInfo)
      {
           // Get original from database or repository
           // Save and validate null 
           // Store relevant information in _errors;
           // Change state
      }
}

Answer 8

Even though I would have loved throwing exception, here it is not appropriate because you are not in fail-fast. You are taking corrective measures on non fatal cases. You just want higher layers to know about it.

public OperationResult<DataSet> Update(MyThing thing, OperationResult<DataSet> papa)
{
    // Either you have a result object from enclosing block or you have null.
    var result = OperationResult<DataSet>.Create(papa);

    if (thing.Id == null) return result.Fail("id is null");

    OtherThing original = _repository.GetOtherThing(thing.originalId);
    if (original == null) return result.warn("Item already deleted");

    if (AnyPropertyDiffers(thing, original))
    {
        UpdateThing(thing, original, result));
        // Inside UpdateThing, take result in papa and do this dance once:
        // var result = OperationResult<DataSet>.Create(papa);
    }

    UpdateThingChildren(thing, result);
    // same dance. This adds one line per method of overhead. Eliminates Merge thingy

    _repository.Recalculate(thing, result); 

    return result.ok();
}

You can eliminate passing result everywhere using Scope pattern from @BigShot, but i personally do not like ambient contexts. The could be anything you may need to return back.

class OperationResult<T> {
    enum SuccessLevel { OK, WARN, FAIL }

    private SuccessLevel _level = SuccessLevel.OK;
    private List<String> _msgs = new ...

    public T value {get; set};

    public static OperationResult<T> Create(OperationResult<T> papa) {
        return papa==null ? new OperationResult<T>() : papa;
    }

    public OperationResult<T> Fail(string msg) {
        _level = SuccessLevel.Fail;
        _msgs.add(msg);
        return this; // this return self trick will help in reducing many extra lines in main code.
    }

    // similarly do for Ok() and Warn()

}