10
\$\begingroup\$

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?

\$\endgroup\$
1
  • 7
    \$\begingroup\$ I think it would be more natural to return a generic response object (Response<UserDTO>) that contains the DTO result as a property. It just seems more straightforward to reason about a response that has the result that you want, rather than returning a result then checking the response to see if the result actually has anything. \$\endgroup\$ Jan 25, 2014 at 4:07

1 Answer 1

5
\$\begingroup\$

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);
}
\$\endgroup\$
1
  • \$\begingroup\$ +1 In Java I tend to offer two versions of any single-object finder method: findByFoo returns null when none match whereas getByFoo throws an exception. This gives the client programmer the freedom to handle a missing item gracefully or fail as needed. While the two methods require a little more code, they can greatly increase DRY in many cases. \$\endgroup\$ Feb 14, 2014 at 3:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.