Telling a client that there is no result via the DTO pattern

Question

This is how I handle this situation right now:

For example, I have service that returns UserDto by user ID: GetUserById.

• Service never returns null or throws exceptions.
• Service always returns DTO objects derived from the base class (DtoBase).
• Each DTO object relates to some Entity type. Each entity has an assigned ID (In my example I used long for IDs, but the Response class can be made generic).

DtoBase class:

[DataContract]
public abstract class DtoBase
    : IDtoResponseEnvelop
{
    [DataMember]
    private readonly Response _responseInstance = new Response();

    //This constructor should be called when there is no result
    protected DtoBase()
    {}

    //Each DTO object relates to some Entity (each entity has as ID). 
    //And if there is some result this constructor should be called. 
    protected DtoBase(long entityId)
    {
        _responseInstance = new Response(entityId);
    }

    #region IDtoResponseEnvelop Members

    public Response Response
    {
        get { return _responseInstance; }
    }

    #endregion
}

Basically, the Response class aggregates operation response information such as: if there is any value, exception and warnings:

[DataContract]
public class Response
{        
    #region Constructors

    public Response():this(0){}

    public Response(long entityId)
    {
        _entityIdInstance = entityId;
    }

    #endregion        

    #region Private Serializable Members

    [DataMember]
    private BusinessExceptionDto _businessExceptionInstance;

    [DataMember]
    private readonly IList<BusinessWarning> _businessWarningList = new List<BusinessWarning>();

    [DataMember]
    private readonly long _entityIdInstance;

    #endregion

    #region Public Methods

    public void AddBusinessException(BusinessException exception)
    {
        _businessExceptionInstance = new BusinessExceptionDto(exception.ExceptionType, exception.Message, exception.StackTrace);
    }

    public void AddBusinessWarnings(IEnumerable<BusinessWarning> warnings)
    {
        warnings.ToList().ForEach( w => _businessWarningList.Add(w));
    }

    #endregion

    #region Public Getters

    public bool HasWarning
    {
        get { return _businessWarningList.Count > 0; }
    }

    public IEnumerable<BusinessWarning> BusinessWarnings
    {
        get { return new ReadOnlyCollection<BusinessWarning>(_businessWarningList); }
    }

    public long EntityId
    {
        get { return _entityIdInstance; }
    }

    public bool HasValue
    {
        get { return EntityId != default(long); }
    }

    public bool HasException
    {
        get { return _businessExceptionInstance != null; }
    }

    public BusinessExceptionDto BusinessException
    {
        get { return _businessExceptionInstance; }
    }

    #endregion
}

Now, having this in place, on server side we have:

    private UserDto GetUserByIdCommand(IRepositoryLocator locator, long userId)
    {
        var item = locator.GetById<User>(userId);
        if (item != null) 
             return Mapper.Map<User, UserDto>(item); //mapper calls UserDto(item.ID) constructor

        _businessNotifier.AddWarning(BusinessWarningEnum.Operational,
            string.Format("User with Id=\"{0}\" not found", userId));
        return new UserDto(); //no Entity id is provided here, since no value available
    }

And, on client side:

    var userDto = UserService.GetUserById(1);
    if(!userDto.Response.HasValue)
    {
          //No result is available
    }

Is my approach fine or is there a better way?

Answer 1

I only quickly glanced at your code, so this isn't going to be a thorough review, but I think the basic idea is surprising:

Service never returns null or throws exceptions.

I think this approach is error-prone. When I call a method called GetUserById, for an Id that doesn't exist, I expect either an ArgumentOutOfRangeException (or similar), or a null return value - the last thing I expect is a valid object filled up with default values filling up all members.

As @jlnorsworthy mentioned in his excellent comment, your approach is far from instinctive. Instead of:

var userDto = UserService.GetUserById(1);
if(!userDto.Response.HasValue)
{
      //No result is available
}

I would perfer to have:

var userResult = UserService.GetUserById(1);
if(userResult.Result == null)
{
      //No result is available
}

Also HasValue confusing because it is a well-known member of Nullable<T>, so I'd consider changing EntityId to be Nullable<long> or Nullable<Int64>:

public bool HasValue
{
    get { return EntityId.HasValue; }
}

---

What is IRepositoryLocator? If it is what I think it is, you should read up on Mark Seeman's blog. Shortly put:

Service Locator is a well-known pattern, and since it was described by Martin Fowler, it must be good, right?

No, it's actually an anti-pattern and should be avoided.

---

Little nitpick, I wouldn't use ForEach here:

public void AddBusinessWarnings(IEnumerable<BusinessWarning> warnings)
{
    warnings.ToList().ForEach( w => _businessWarningList.Add(w));
}

It would be much more readable (and less semantically controversial) to write it like this:

public void AddBusinessWarnings(IEnumerable<BusinessWarning> warnings)
{
    _businessWarningList.AddRange(warnings);
}