Generic Wrapper class for HTTPClient methods

Question

I am trying to using HttpClient for making API calls from a different domain and for that purpose I have created the following service class after doing some research. It is 4.7.2 framework and not a .NET core system. I have created library class for this with following classes. Please give me your valuable suggestions for changes or enhancements

IRequestService.cs file

public interface IRequestService
    {
        Task<TResult> GetAsync<TResult>(HttpClientNS client, string url);
        Task<string> GetStringAsync(HttpClientNS client, string url);
        Task<TResult> PostAsync<TRequest, TResult>(HttpClientNS client, TRequest t, string url);
        HttpClientNS GetClient();
    }

RequestService.cs

public class RequestService : IRequestService
    {
    private readonly string _hostWebApiUrl;
    private readonly string _apiAuthKey;

    public RequestService()
    {
        _hostWebApiUrl = Config.AppSettings["TestWebApiURL"];
        _apiAuthKey = Config.AppSettings["ApiKey"];
    }

    public RequestService(string hostWebApiUrl, string apiAuthKey)
    {
        _hostWebApiUrl = hostWebApiUrl;
        _apiAuthKey = apiAuthKey;
    }
    public HttpClientNS GetClient()
    {
        string _baseAddress = _hostWebApiUrl;
        var client = new HttpClientNS
        {
            BaseAddress = new Uri(_baseAddress)
        };

        client.DefaultRequestHeaders.Add("AuthApiKey", _apiAuthKey);
        return client;
    }

    public async Task<TResult> GetAsync<TResult>(HttpClientNS client, string url)
    {
        client.Timeout = TimeSpan.FromSeconds(30);
        var result = default(TResult);
        try
        {
            var response = await client.GetAsync(url);
            response.EnsureSuccessStatusCode();
            //var jsonString = await response.Content.ReadAsStringAsync();
            //var result = JsonConvert.DeserializeObject<TResult>(jsonString);


            await response.Content.ReadAsStringAsync().ContinueWith((Task<string> x) =>
            {
                if (x.IsFaulted)
                    throw x.Exception;

                result = JsonConvert.DeserializeObject<TResult>(x.Result);
            });



        }

        catch (Exception ex)
        {

        }
        return result;
    }

    public async Task<string> GetStringAsync(HttpClientNS client, string url)
    {
        var httpRequest = new HttpRequestMessage(new HttpMethod("GET"), url);
        var response = client.SendAsync(httpRequest).Result;
        var jsonString = await response.Content.ReadAsStringAsync();
        return jsonString;
    }


    public async Task<TResult> PostAsync<TRequest, TResult>(HttpClientNS client, TRequest t, string url)
    {
        var result = default(TResult);
        var json = JsonConvert.SerializeObject(t);
        HttpContent httpContent = new StringContent(json);
        httpContent.Headers.ContentType = new MediaTypeHeaderValue("application/json");

        //response.EnsureSuccessStatusCode();
        //var jsonString = await response.Content.ReadAsStringAsync();
        //var result = JsonConvert.DeserializeObject<TResult>(jsonString);
        //return result;  


        var response = await client.PostAsync(url, httpContent).ConfigureAwait(false);

            response.EnsureSuccessStatusCode();

            await response.Content.ReadAsStringAsync().ContinueWith((Task<string> x) =>
            {
                if (x.IsFaulted)
                    throw x.Exception;

                result = JsonConvert.DeserializeObject<TResult>(x.Result);

            });
        

        return result;

    }

}

I am making calls to these wrapper class containing generic methods as follows:

public class TestApiClient
    {

        private IRequestService RequestService;
        private readonly string _apiUrl;
        public TestApiClient(IRequestService request)
        {
            RequestService = new RequestService();
            _apiUrl = "api/TestCustomer";
        }       

        public async Task<bool> IsCustomerVendor(int vendorId)
        {

            var url = string.Format(_apiUrl + "/IsCUstomerVendor?vendorId={0}", vendorId);
            var client = RequestService.GetClient();
            var result = await RequestService.GetAsync<bool>(client, url);

            return result;
        }



        public async Task<bool> IsCUstomerDataInvalid(int productId, int financeCompanyId, string balloonDealerState)
        {
             

            var url = _apiUrl + "/IsCUstomerDataInvalid";
            var content = new { CustomerId = customerId, CompanyId = companyId, DealerState = DealerState };
            //var result = _webClientHelper.PostContent<bool>(url, content);
            var client = RequestService.GetClient();
            var result = await RequestService.PostAsync<Object,bool>(client, content, url);

            return result;
        }

    }

Answer 1

Quick remarks:

• Don't do this:

   public RequestService()
   {
       _hostWebApiUrl = Config.AppSettings["TestWebApiURL"];
       _apiAuthKey = Config.AppSettings["ApiKey"];
   }
  
   public RequestService(string hostWebApiUrl, string apiAuthKey)
   {
       _hostWebApiUrl = hostWebApiUrl;
       _apiAuthKey = apiAuthKey;
   }
  

  Instead pass the values you get to the constructor that can accept these values:

   public RequestService()
           : this(Config.AppSettings["TestWebApiURL"],
                   Config.AppSettings["ApiKey"])
   {
   }
  

  I'd also concentrate all the code that retrieves values from Config.AppSettings in a single class instead of littering my code with calls to Config.AppSettings.

• Is it a good idea to be inconsistent with names? "AuthApiKey" vs _apiAuthKey is somewhat confusing to me, for instance. (And then the app setting key is "ApiKey", yet another name.)

• It's a bad idea to catch an exception and not do anything with it. I'd hope to see at least some logging.

• Why not use the Get property of HttpMethod?

• GetStringAsync doesn't catch errors or exceptions. What if something goes wrong? The same is true for many of your other methods which connect to the API. I'd provide a way to log all those things instead of expecting everything to go well; hunting for problems when things don't go as planned and finding there is no logging is very frustrating.

  I'd go even further and offer logging for all kinds of things: which method is called with what value, etc. That way you can see the flow that happened and you can likely easier spot why something went wrong (because sometimes merely logging the error is insufficient).

• Why do you have IRequestService request (bad name! this isn't a "request") as a parameter of TestApiClient, yet don't use it?

• Why do you use the old string.Format instead of string interpolation?

• Is the "u" in IsCUstomerDataInvalid (both the method name and the URL used inside that method) supposed to be a "U"? Or is that a typo?