What type of result should be returned from the service layer?

Question

Say, for example, that I have a PostsService that creates a post:

public class PostsService : IPostsService
{
    public bool Create(Post post)
    {
        if(!this.Validate(post))
        {
            return false;
        }

        try
        {
            this.repository.Add(post);
            this.repository.Save();
        }
        catch(Exception e)
        {
            return false;
        }
    }
}

The problem with this is that if an exception is thrown during the repository actions, it's swallowed. Create() returns false and all that the consumer knows is that the Post wasn't added, but doesn't know why.

Instead, I was think of having a ServiceResult class:

public class ServiceResult
{
    public bool Success { get; private set; }
    public Exception Exception { get; private set; }
}

Would this be a good design? Or do I even need to report these errors to the consumer? Is it sufficient to say "An error occurred while adding the post." and then log the exception inside of the service?

Any other suggestions are appreciated.

Answer 1

I think it depends on the scope of why. Some failure reasons shouldn't concern the consumer, in my opinion and these should be logged in the service. On the other hand, there are cases when the consumer should be informed of these reasons, so in addition to logging them, you should indicate the cause of the error somehow.

For instance, suppose a very common use case of a user registration service. It's quite likely that you'd have a unique attribute that identifies the user (user handle, email, etc). This uniqueness must be enforced by the service (and probably at a database level too). Now, if the consumer is trying to register a user and it fails because the constrain was violated, the consumer should be notified of this reasons (so it could try some other user handle). Failed validations fall in the same scenario, most of the times.

If there's some kind of internal DB problem on the other hand, the consumer should not be informed of the details (because that's exclusive responsibility of the service; and it's not something the consumer can do anything about anyway).

Considering that, I'd say returning a boolean is insufficient in many cases. So having some kind of ServiceResult type doesn't sound like a bad idea. I'm not sure I would include the Exception though. But perhaps you can create some kind of ServiceExpection, specific to your service. Sometimes error codes are just fine for this, too.

Answer 2

It's a really bad idea to catch all exceptions. You only catch that which you can reasonably handle. You can't handle all exceptions, so why are you catching it? To prevent a stack trace?

If you don't want your users to see a horrible stack trace, I get that, but you do want it to die and die horribly so that something isn't corrupted.

Let's say that service threw an OutOfMemoryException, you just caught it, and won't know whether or not your application is leaking memory like a sieve or is improperly allocating large objects.

That's a bad thing.

If you don't want your user to see what was wrong, then you should change your catch to say:

catch (Exception e)
{
    //send it to yourself, log it. Just don't swallow it.
    LogErrorAndEmailDevTeam(e); 
    throw new SaveFailedException("We're sorry. \n" + 
    "It's not your fault, but something bad happened.\n\n" + 
    "We've been notified and will fix it as soon as possible.");
}

But even better would be to let the application die a horrible death instead of catching that exception so that you know quickly when something goes wrong.

You don't want your application to continue in a corrupted state, but that's what catch (Exception) does. Are you really sure you can handle whatever is thrown?