Review of a generic request handler class (RestSharp)

Question

I wrote a class that uses RestSharp to access Rest and HTTP API web services. I tested it and it works however I was wondering if any changes could be made to it to make it better.

public class RequestHandler
{
 //Client to translate Rest Request intot HTTP request and process the result.
 private RestClient client;
 //this property stores the base URL
 const string baseUrl = "";
 //stores the account id and secret key if they will be needed for any authentication processes.
 readonly string account_id;
 readonly string secretKey;
 //this property stores the API key
 readonly string accessKey = "";
 //This constructor contains the account id and secret key that will be used for authorization when connecting to the API
 public RequestHandler(string account_id,string secretKey)
 {
 client = new RestClient(baseUrl);
 this.account_id = account_id;
 this.secretKey = secretKey;
 }
 //The default constructor 
 public RequestHandler()
 {
 client = new RestClient(baseUrl);
 }
 public void Execute<T>(int id,Action<T> Success, Action<string> Failure) where T:new()
 {
 RestRequest request = new RestRequest();
 request.RequestFormat = DataFormat.Json;
 request.AddParameter("id",id,ParameterType.GetOrPost);
 client.ExecuteAsync<T>(request,(response) =>
 {
 if(response.ResponseStatus == ResponseStatus.Error)
 {
 Failure(response.ErrorMessage);
 }
 else
 {
 Success(response.Data);
 }
 });
 }
}

• Ron Klein: I don't see where secret key and account id are used other than assigning them to read only fields in the class.
• dreza: I would probably consider at least chaining your constructors so only one of them will actually do the work. In this case I would probably make your parameterless constructor call the other i.e. RequestHandler() : this(string.Empty, string.Empty)

Answer 1

Depend on the IRestClient interface rather than the RestClient implementation and make the field readonly as it is only set via the constructor:

private readonly IRestClient client;

Invert your dependancy on RestClient, if you are not using an IOC container, at least implement poor mans injection so that the class is testable (e.g. you can supply a mock IRestClient to test RestHandler.

public RequestHandler() // If you are using an IOC container, remove the default constructor.
 : this(new RestClient(baseUrl))
{
}
public RequestHandler(IRestClient restClient)
{
 this.client = restClient;
}

As Ron mentioned above, you don't appear to be using secret key and account id so either remove them if they are not needed or use them.

Making the base url a constant inside the class makes it difficult to re-use, I would move that into the application configuration.

If you do need account_id, remove the underscore and camel case it accountId to adhere to the standard naming conventions.

Adding a property for the DataFormat will make the class more re-usable if you want to call a web service which doesn't support Json:

public DataFormat DataFormat { get; set; }
// Default to Json in the constructor
this.client = restClient
this.DataForma = DataFormat.Json;

ExecuteAsync returns a RestRequestAsyncHandle which allows the request to be aborted, it might be worth looking at whether you need the ability to abort requests or not. You could update your Execute to return the wait handle.

The error checking here doesn't account for any of the other reasons for failure such as a timeout or abort.

if (response.ResponseStatus == ResponseStatus.Error)

Update it to something like this:

switch (response.ResponseStatus)
{
 ResponseStatus.Completed:
 Success(response.Data);
 break;
 ResponseStatus.Error:
 ResponseStatus.TimedOut:
 Failure(response.ErrorMessage);
 break;
}

• hellboy: Should we also use IRestRequest?