Architecture – Should DTO have validators

Architecturedtoprogramming practices

I believe that DTO should be as dummy and easy as possible. However, after hint made by my friend, opportunity to validate data that is set in DTO sounds tempting.

Let's have following example:

//this class only holds bunch of objects from the future

class FooFuture{

    /** @var ArrayObject */
    $collection;

    (other fields ommited for readability)

    /**
     * @param ArrayObject $collection
     */
    public function __construct(ArrayObject $collection = null){

         if(null === $collection){
             $this->collection = new ArrayObject();
         } else {
             $this->collection = $collection;
         } 
    }

    /**
     * @param ArrayObject $col
     */
    public function setCollection(ArrayObject $col){

        $now = new DateTime();        

        foreach($col as $element){
            if($element->getDateTime() < $now){
                throw new InvalidArgumentException('Collection should only contain objects from the future');
            }
        }
    }

    /** 
     * @return ArrayObject
     */
    public function getCollection(){
         return $this->collection;
    }

}

What do you think?

Best Answer

While i agree that it can be usefull to have validation for dto-s i think that dto-s should be as simple as possible.

You can achive boths if you are using a programming language that supports Interfaces like java or c# and put the validation logic into a seperate class that consumes interfaces.

Example

public interface IOrder { ... }

public class OrderDTO implements IOrder { ... }

public class OrderBuisinessObject implements IOrder { ... }

public class OrderValidator {
   void validate(IOrder anOrder) { ... }
}

To answer your question: yes to have validation but not in dto

Related Solutions

Design Patterns – Avoiding Tight Coupling with Distributed State Checks

If you have too many dependencies being passed around, the general technique is to eliminate those dependencies higher up in the call stack, by changing the order decisions are made. This is easiest to explain with an example:

getPath(config, user) {
  if (config.isB2B())
    return b2bpath(config, user);
  else
    return b2cpath(config, user);
}

b2bpath(config, user) {
  if (!config.allowedToAccessPath(user))
    return accessDeniedPage();
  else
    return "My fancy b2b page";
}

b2cpath(config, user) {
  if (!config.allowedToAccessPath(user))
    return accessDeniedPage();
  else
    return "My fancy b2c page";
}

You are repeating the authorization check down at the lowest levels of the call stack, so move it up:

getPath(config, user) {
  if (!config.allowedToAccessPath(user))
    return accessDeniedPage();

  if (config.isB2B())
    return b2bpath();
  else
    return b2cpath();
}

b2bpath() {
    return "My fancy b2b page";
}

b2cpath() {
    return "My fancy b2c page";
}

Then repeat to see if you can move some of the decisions into the code that calls getPath. This is a simple example, but I see the former kind of code all over the place with more layers. Start with decisions at your lowest layers, and try to figure out ways to move them up. Sometimes this requires judicious use of inheritance, like:

getPath(config, user) {
  module = config.isB2B() ? B2BModule() : B2CModule()
  if (!config.allowedToAccessPath(user))
    return module.accessDeniedPage();

  return module.getPath();
}

It's very rare not to be able to simplify dependencies this way. It's a matter of trying different arrangements until you find one that works.

If you have workflow-type dependencies, not just data dependencies, as in your first example, you can separate them out using something like this:

step1 = new ValidateHeaderId(inputHeaderId);
step2 = new FindShipment(retShipmentRepository);
step3 = new ValidateUserPermission(user, inputPostCode, inputEmail);
step4 = new SaveReturnOrder(inputLines, inputReference);
step5 = new CheckSaveStatus();
notSet = new NotSetPage(templateEngine, searchPage);
notPermitted = new NotPermittedPage(templateEngine, searchPage);
saveErrors = new SaveErrorsPage();
success = new SuccessPage();
requestTokenConfirmation = new RequestTokenConfirmation();

steps = [step1, [notSet, step2], [notSet, step3], [notPermitted, step4],
         [saveErrors, step5], [success, requestTokenConfirmation]];
executeSteps(steps);

This recognizes you have a series of steps which each produce some sort of result and choose the next step. executeSteps abstracts away the repetition of calling run() on each step, and passing the output from the previous step into the next step. This allows the steps to be stored in a data structure instead of a function, which can then be built up in several different ways, including by some sort of registration process or config file. Once each step object has been created, its dependencies no longer need to be tracked outside it. I believe the rules engines from BobDalgleish's answer are basically pre-existing libraries to help you do this more easily.

C# – How to Model a Multi-Channel Communication Device

I believe you should keep the amount of channels abstracted:

The amount of channels may vary in the future, e.g. hardware up/downgrade (new devices)
You may want to reserve some channels in the future, e.g. for out-of-band communications and notifications

The exactness should be kept and ensured in initialization.

You should expose the channels with the least specific type possible, such as IEnumerable<ReceivingChannel> and IEnumerable<TransmittingChannel>.

The MainDevice should expose a way to initialize or manage these collections, you have several non-exclusive options:

Constructor parameters

private List<IReceivingChannel> receivingChannels = new List<IReceivingChannel>();

private List<IReceivingChannel> transmittingChannels = new List<ITransmittingChannel>();

public MainDevice(IEnumerable<IReceivingChannel> receivingChannels,
                  IEnumerable<ITransmittingChannel> transmittingChannels)
{
    this.receivingChannels.AddRange(receivingChannels);
    this.transmittingChannels.AddRange(transmittingChannels);
}

Fluid initialization methods

public MainDevice WithReceivingChannels(IEnumerable<IReceivingChannel> receivingChannels)
{
    this.receivingChannels.AddRange(receivingChannels);
    return this;
}

public MainDevice WithReceivingChannels(params IReceivingChannel[] receivingChannels)
{
    return WithReceivingChannel((IEnumerable<IReceivingChannel>)receivingChannels);
}

public MainDevice WithTransmittingChannels(IEnumerable<ITransmittingChannel> transmittingChannels)
{
    this.transmittingChannels.AddRange(transmittingChannels);
    return this;
}

public MainDevice WithTransmittingChannels(params ITransmittingChannel[] transmittingChannels)
{
    return WithTransmittingChannels((IEnumerable<ITransmittingChannel>)transmittingChannels);
}

Perhaps throw an exception (e.g. InvalidOperationException) in case you use these methods after the object has been first used by actual worker methods. Or better yet, have these methods in a builder class, where you finally call GetDevice which will create and initialize the device, possibly with dependency injection (I will not discuss DI here.)

Methods

AddReceivingChannel, RemoveReceivingChannel, AddTransmittingChannel, RemoveTransmittingChannel

GetReceivingChannels returning IEnumerable<IReceivingChannel>, or a similar ReceivingChannels property

GetTransmittingChannels returning IEnumerable<ITransmittingChannel>, or a similar TransmittingChannels property

GetReceivingChannelById, GetReceivingChannelByAddress, GetTransmittingChannelById, GetTransmittingChannelByAddress

Don't use settable properties, as that is asking for trouble. The MainDevice should be the only entity responsible for managing its actual composition.

This way, you may start by using arrays or lists to store the channels, but switch to dictionaries or trees (e.g. SortedDictionary<TKey, TValue>) if accessing them by some key becomes a bottleneck. And the users of MainDevice are just as happy.

You can define a base interface IChannel for the common things that all channels do, such as opening and/or closing with an optional timeout, and have IReceivingChannel and ITransmittingChannel hold the specific operations, such as receiving and transmitting with an optional timeout:

public interface IChannel
{
    void Open(TimeSpan timeout);
    void Close(TimeSpan timeout);
}

// I'm using generics, but you don't have to if you'll always
// deal with the same data type, e.g. IEnumerable<byte>
//
// I'm also over-simplifying by not stating end-of-transmission
// and other usual channel state.

public interface IReceivingChannel<out T> : IChannel
{
    T Receive(TimeSpan timeout);
}

public interface ITransmittingChannel<in T> : IChannel
{
    void Transmit(T obj, TimeSpan timeout);
}

In case you need to access some inner object from a channel, define specific interfaces on which you may use the as operator instead of the actual class. For instance, a IFrobberContainer interface with a GetNativeFrobber method (or similar NativeFrobber property), which does not extend IReceivingChannel.

This way, any object can contain a frobber:

public interface IFrobberContainer
{
    IFrobber NativeFrobber { get; }
}

internal class FrobberReceivingChannel : IReceivingChannel<Thing>, IFrobberContainer
{
    public void Open(TimeSpan timeout) { /* ... */ }

    public void Close(TimeSpan timeout) { /* ... */ }

    public Thing Receive(TimeSpan timeout) { /* ... */ }

    public IFrobber NativeFrobber { get; private set; }
}

internal class FrobberTransmittingChannel : ITransmittingChannel<Thing>, IFrobberContainer
{
    public void Open(TimeSpan timeout) { /* ... */ }

    public void Close(TimeSpan timeout) { /* ... */ }

    public void Transmit(Thing obj, TimeSpan timeout) { /* ... */ }

    public IFrobber NativeFrobber { get; private set; }
}

In case you have something extra to do with frobber channels, you can ask if the channel has a frobber using the as operator.

With the new C# 6 null-conditional operator, you can do this:

(channel as IFrobberContainer)?.NativeFrobber?.Frobulate();

instead of this:

var frobberContainer = channel as IFrobberContainer;
if (frobberContainer != null)
{
    var frobber = frobberContainer.NativeFrobber;
    if (frobber != null)
    {
        frobber.Frobulate();
    }
}

By segregating these distinct aspects into interfaces, you make your actual objects more flexible, more easily replaceable and mockable (good for unit testing).

If you insist on having 10 receiving channels and 4 transmitting channels, then you probably shouldn't use collections, but conversely use very good names for 14 properties. For generic iteration, you can keep internal collections initialized once, to avoid obvious code repetition.

You should still use interfaces and document which ones each channel implements, instead of using actual classes as the types for these properties, to retain the replaceability benefits for refactoring, maintenance and testing purposes.

Best Answer

Related Solutions

Design Patterns – Avoiding Tight Coupling with Distributed State Checks

C# – How to Model a Multi-Channel Communication Device

Related Topic