2
\$\begingroup\$

We are designing a small framework to create and send messages to external parties as a result of internal events. I am reasonably happy with the design, but I am wandering if it can be further improved.

The external messages are large and complex and involve getting data from different areas in the system and are sent asynchronously when the internal event happens. A single internal event can cause multiple external messages to be sent.

The internal event (incomingMessage) contains some of the information required to populate the message, and that is provided to the message sending framework, but additional information is required to be retrieved from the database through a potentially expensive query.

The MessageBuilder class builds the messages resulting from the internal event with the help of a list of IMessageContentProvider, which can provide content for different parts of the message. These are passed in through the constructor.

public interface IMessageBuilder<in TIncoming>
{
    IEnumerable<object> Build(TIncoming incomingMessage);
}
 public class MessageBuilder<TIncoming, TOutgoing> : IMessageBuilder<TIncoming> where TOutgoing : class, new()
{
    private readonly IMessageContentProvider<TIncoming, TOutgoing>[] contentProviders;

    public MessageBuilder(IMessageContentProvider<TIncoming, TOutgoing>[] contentProviders)
    {           
        this.contentProviders = contentProviders;
    }

    public IEnumerable<object> Build(Project project, IServiceBusResolver serviceBusResolver, TIncoming incomingMessage)
    {        
        TOutgoing outgoingMessage = BuildOutgoingMessage(project, serviceBusResolver, incomingMessage);

        return outgoingMessage != null ? new[] { outgoingMessage } : null;
    }

    protected virtual TOutgoing BuildOutgoingMessage(Project project, IServiceBusResolver serviceBusResolver, TIncoming incomingMessage) 
    {
        TOutgoing outgoingMessage = new TOutgoing();

        foreach (IMessageContentProvider<TIncoming, TOutgoing> contentProvider in contentProviders)
        {
            contentProvider.Populate(incomingMessage, ref outgoingMessage);
        }

        return outgoingMessage;
    }

The external message definition also requires some translation from internal types to external message types, these are enumerations and data structures used in the external large message. This is done in a static converter class (this seems a bit clunky), which has a series of methods to convert specific parts of the message. All required data is passed in as parameters.

public static class XXMessageConverter
{
    public static TypeA GetTypeA(InternalTypeA internalTypeA, OtherData1 data, OtherData2 data2)
    {
    }
    // ... More getter methods
}

One thing that worries me is that the content providers are independent of each other and there is nothing stopping one provider overwriting data populated by another. Also the order of the providers passed in to the MessageBuilder is important. I have been thinking whether using the fluent builder pattern will be more appropriate and whether message type conversion should happen in the builder or in a separate class.

I added a new type ITypeConverter and started adding a series of converters for translating messages. It's simple at this stage, I may have to add some context to it as some messages require external information to convert.

public interface ITypeConverter<in TSource, out TDestination>
{
    TDestination Convert(TSource source);
}
\$\endgroup\$
  • \$\begingroup\$ What you've posted looks good to my eye, but you seem to also be concerned about the static class that you've not really shown us. You may get a better review if you actually show us the static class you're concerned about. \$\endgroup\$ – RubberDuck Dec 29 '14 at 11:40
3
\$\begingroup\$
public IEnumerable<object> Build(Project project, IServiceBusResolver serviceBusResolver, TIncoming incomingMessage)
{        
    TOutgoing outgoingMessage = BuildOutgoingMessage(project, serviceBusResolver, incomingMessage);

    return outgoingMessage != null ? new[] { outgoingMessage } : null;
}

Why should this return an IEnumerable<T> ? This just doesn't make sense at all. BuildOutgoingMessage() is only building one object.

The BuildOutgoingMessage() method doesn't use the serviceBusResolver parameter. If this is because it is only for overriding in a derived class, you should consider to add some documentation for this case.

The MessageBuilder class states that it implements IMessageBuilder<TIncoming> but it doesn't implement its method.

\$\endgroup\$
  • \$\begingroup\$ Good points, the serviceBusResolver was removed from all other parts of the code to make the example clearer, I just missed a couple of spots. \$\endgroup\$ – Ned Stoyanov Dec 29 '14 at 22:46

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.