3

I've implemented a command pattern in a project I'm working on. This is pretty much the current structure:

public class Response
{
    public bool Success { get; private set; }

    public static Response CreateErrorResponse()
    {
        return new Response { Success = false };
    }
}

public interface ICommand<T> where T : Response
{
    Task<T> ExecuteAsync();
}

public abstract CommandBase : ICommand<T> where T: Response
{
    protected abstract Uri BuildUrl();
    protected abstract Task<T> HandleResponseAsync();

    public async override Task<T> ExecuteAsync()
    {
        var url = BuildUrl();
        var httpClient = new HttpClient();

        var response = await httpClient.GetAsync(url);
        return await HandleResponseAsync(response);
    }
}

I want to handle any exceptions that could be thrown by the HttpClient, so I want to change CommandBase.ExecuteAsync to something like this...

public async override Task<T> ExecuteAsync()
{
    var url = BuildUrl();
    var httpClient = new HttpClient();

    try
    {
        var response = await httpClient.GetAsync(url);
        return await HandleResponseAsync(response);
    }
    catch (HttpRequestException hex)
    {
        return Response.CreateErrorResponse(); // doesn't compile
    }
}

The compile error I get is "Cannot convert type Response to async return type T". I can't use T.CreateErrorResponse(), as outlined in this question.

How can I work around this?

Edit to downvoters: whether or not you agree with catching exceptions in a library like this, the question still stands!

Community
  • 1
  • 1
rikkit
  • 1,127
  • 3
  • 18
  • 35
  • 2
    The `CreateErrorResponse()` is of type `IResponse` but expected `Response`. – Hamlet Hakobyan Sep 01 '13 at 20:45
  • What's the idea behind this? You could just let it throw the exception, and it'll automatically be aggregated to the awaiter. – Claus Jørgensen Sep 01 '13 at 21:44
  • @ClausJørgensen true. But in this specific case IMO it's neater to treat HttpRequestExceptions the same as errors on the API the library is wrapping, since the call can fail for other reasons (overloaded service, 404, etc). I'm going to add a flag to disable/enable the behaviour if the consumer wants the exceptions though. – rikkit Sep 01 '13 at 21:58
  • Wrap the exceptions then. You shouldn't wrap exceptions like this, it's a really bad API design. When using Tasks, you can explicitly check for the .Error property for getting the exception when it fails. You're preventing the consumer from using tasks correctly by wrapping it like this. – Claus Jørgensen Sep 01 '13 at 22:09
  • I understand your rationale - separation of concerns, single responsibility - but I am developing an application in parallel to my library. Working with my library, I've found myself wanting this as an option. Client code already checks the Success property on the Response objects returned from the library, so it's neater to not require try/catch code everywhere. This is also the only case I'm planning to do this. What do you mean when you mention the Error property on Tasks? – rikkit Sep 01 '13 at 22:24
  • @rikkit I kinda agree with Claus. But if you have good reasons to do it this way, at least modify the interface for errors a bit, so that the exception can be retrieved, as in `CreateErrorResponse(ExceptionDispatchInfo errorExceptionInfo)` and provide this as a property next to the `Success` property. In fact I will update my answer to include this. – Alex Sep 01 '13 at 22:31
  • Thanks @Alex for the suggestion - I'll change from the response codes to that once I've got this problem solved :) – rikkit Sep 01 '13 at 22:34

2 Answers2

3

Although I am not sure this is the best solution (or feasible in your specific use case), what you can do is:

public class Response
{
    public bool Success { get; private set; }
    public ExceptionDispatchInfo ErrorInfo { get; private set; }
    public bool HasFailed
    {
        get { return !Success; }
    }

    public static T CreateErrorResponse<T>(ExceptionDispatchInfo errorInfo) where T : Response, new()
    {
        var response = new T();
        response.Success = false;
        response.ErrorInfo = errorInfo;
        return response;
    }
}

Usage:

catch (HttpRequestException hex)
{
    return Response.CreateErrorResponse<T>(ExceptionDispatchInfo.Capture(hex)); // should compile (I did not check)
}
Alex
  • 13,024
  • 33
  • 62
0

You can cast the response to T. EDIT: Added full source code

public class Response
{
    public bool Success { get; private set; }

    public static Response CreateErrorResponse()
    {
        return new Response { Success = false };
    }
}

public interface ICommand<T> where T : Response
{
     Task<T> ExecuteAsync();
}

public abstract class CommandBase<T> : ICommand<T> where T: Response
{
    protected abstract Uri BuildUrl();
    protected abstract Task<T> HandleResponseAsync();

    public async Task<T> ExecuteAsync()
{
    var url = BuildUrl();
    var httpClient = new System.Net.Http.HttpClient();

    try
    {
        var response = await httpClient.GetAsync(url);
        return null;// await HandleResponseAsync(response);
    }
    catch (Exception hex)
    {
        return (T)Response.CreateErrorResponse(); // doesn't compile
    }
}
}

public async override Task<T> ExecuteAsync()
{
    var url = BuildUrl();
    var httpClient = new HttpClient();

    try
    {
        var response = await httpClient.GetAsync(url);
        return await HandleResponseAsync(response);
    }
    catch (HttpRequestException hex)
    {
        return (T)Response.CreateErrorResponse(); // compiles on liqpad
    }
}
NeddySpaghetti
  • 13,187
  • 5
  • 32
  • 61
  • But the instance you are returning is *not* a `T`. I.e. it will result in a runtime error now (`InvalidCastException`) even though it compiles. I doubt that is an acceptable solution. – Alex Sep 01 '13 at 22:27
  • CreateErrorResponse() returns Response, and T derives from Response - for this to work T needs to be declared as covariant. The Task type isn't covariant though so this would work if I didn't need the async. – rikkit Sep 01 '13 at 22:29