views:

100

answers:

2

I’m trying to refactor some “sending out an email” code by dividing the steps (validate, attach related content, format, send) into separate classes that can be more easily tested, logged, and updated.

As part of this I’ve got to figure out a way for the operations to communicate validation or transient errors (“that item was deleted”) back to the originator so that it can ask the user for more info or tell them the bad news. Here is the path the thread takes (yay, a doodle)

     "Controller"                     
      .   -> Outbox 
      .         -> Validator 
      .         -> Formatter 
      .         -> Sender
      .   <- 

                   -> Parameters, work in progress
                   <- Good, not so good, "you better sit down" news

So you're a thoughtful sort, between “return”, “exceptions”, or “context”… which one makes you happiest?

A. Throw an exception at any problem and let the controller divide up the ones it can handle gracefully and the ones that know my “beeper”.

B. Return some sort of Result<T>class to carry both the product of the operations (an email) and the enumerated results of various operations.

C. Pass a context into/outof all the steps where they can indicate any parameters they can’t deal with and keep the method signatures very straightforward.

D. Son, you’re overthinking this.. Here is what you’re gonna do : <YourSpecialJujuHere/>

Thanks for any and all contributions, you rock in concert.

+1  A: 

Maybe the problem lies in the sequencing of actions, that makes an action call the next.

A different approach is to make the Controller call all actions in turn. In that case, there is a direct relation between your Controller and each action.

Each action may return a simple result, or signal an error via a way appropriate to its case:

  • Exceptional situations via an exception
  • No result via a null return.
  • ...

Reuse from one action to another can happen as local variables.

Sample code (add parameters and so on as needed) :

    class Controller1 {

       private Sender sender = new SenderImpl();

       public void process(String text) {
         try {
           Outbox box = getOutbox();
           List<Errors> errors = validate(text);
           if (!errors.isEmpty()) {
             ....
             return;
           }
           String formatted = format(text);
           sender.send(formatted);
         } catch(MyException e) {
           ....
         }
       }
    }


Although in this code, the steps are delegated to method of the same class, it is very easy to follow the same structure with instances of other classes (but only as needed, don't over-engineer). As you mentionned, this can be justified for testability. I changed the sample code for sender.

KLE
Good point, making actions call their predecessors smells bad and thankfully is just an artifact of my poor doodling skills. So far I have the Outbox encapsulating the steps and calling each in sequence. I also agree that the try{}catch for exceptional cases and inputs/outputs as a combination of references and returns makes sense. The bit I still need to capture is when an operations input is a different type than its output. var email = Formatter(emailParameters);Which means I've got to decide if List<Errors> is part of the return or passed into each.
@james Thanks for you comment. If you need some more specific opinions on a specific part, please let us know.
KLE
+4  A: 

You could use the Template Method pattern with the Strategy pattern:

Your Controller becomes a Template Method. For each step of the email sending process, you call out to a delegate/Strategy class that implements the step.

public class EmailSender
{
    private iOutboxGetter outboxGetter;
    private iMsgValidator validator;
    private iMsgFormatter formatter;
    private iMsgSender    sender;

    //setters for each stragegy, or a constructor
    //good use for IOC container

    public iSendResult SendMessage(iMsgParams params)
    {
        try
        {
            var outbox = outboxGetter.getOutbox(params.outbox);
            var validationResults = validator.validate(params);
            if(validationResults.IsValid)
            {
                var msg = formatter.formatMsg(params.message);
                sender.send(msg);
                return new AllGoodSendResult();
            }
            else
            {
                return new ValidationFailedSendResult(validationResults);
            }
        } 
        catch(CatastrophicException e)
        {
           Pager.SendCriticalPage(e.message);
            return new CatistrophicFailureSendResult(e);
        }
    }
}

I prefer to use exceptions when the code must deviate from the Happy Path. I feel they keep the logic and the error handling cleanly separated.

Edit: The return from the SendMessage method indicates to the caller whether validation passed or not, and what failed in validation. The caller can then prompt the user for more information and retry, or indicate success. An exception is only thrown in the event of a truely exceptional circumstance.

Using this approach, each component of your algorithm can be mocked and tested independently and no Strategy needs to know how any other Strategy works, nor does it need to know how to handle someone else's errors. Finally, all of your error handleing is centeralized in a single place.

Ryan Michela
Cool, that fits really well with the intention of the Outbox, nice to know its got a name. I like the idea of "The Happy Path", but if the validation goes wrong throwing an exception like UserForgotReturnAddress feels "loud". I have this unhelpful desire to see try-again as part of the happy path (option B).
I updated the code to only use exceptions in the case of a truely exceptional circumstance. If validation fails, the user will need to be prompted for more/correct info before trying again.
Ryan Michela
+1 for mixing exception and return codes: I usually throw when something went wrong within MY code (bad assumption for example) and use return codes for user validation.
Matthieu M.