External API calls from a C#.NET client

Question

I have a simple c# mvc client that calls an external api..here is my attempt

 public class ProductController : Controller
{
    private readonly IProductService _productService;

    public ProductController(IProductService quizService)
    {
        _productService = quizService;
    }

    public ActionResult List()
    {
        var viewModel = _productService.GetList(); //gets all 
        return View("List", viewModel);
    }

    public ActionResult Details(string id)
    {
        var viewModel = _productService.GetDetails(id); //get detail 
        return View("Details", viewModel);
    }
}

Below is the implementation details of IProductService

public interface IProductService
{
    List<ProductListResponse> GetList();
    ProductDetailResponse GetDetails(string id);
}

public class ProductService : IProductService
{
    private readonly IClient _httpClient;
    private readonly IConfigurationService _configurationService;
    private const string ExternalApiBaseUrl = "ExternalApiBaseUrl";

    public ProductService(IClient httpClient, IConfigurationService configurationService)
    {
        _httpClient = httpClient;
        _configurationService = configurationService;
    }

    public List<ProductListResponse> GetList()
    {
        var baseUrl = _configurationService.Get(ExternalApiBaseUrl);
        var restClient = _httpClient.GetClient(baseUrl);
        var restRequest = _httpClient.GetRequest();

        var products = _httpClient.ExecuteRequestToGetList(restClient, restRequest) as IList<ProductBase>;
        if (products != null)
        {
            return products.Select(prod=> new ProductListResponse
            {
                Id = prod.Id,
                OnSaleDate = prod.EndDate,
                Name = prod.Name,
                Popularity = prod.Popularity
            }).ToList();
        }
        return new List<ProductListResponse>();
    }

    public ProductDetailResponse GetDetails(string id)
    {
        var baseUrl = _configurationService.Get(ExternalApiBaseUrl);
        var restClient = _httpClient.GetClient(baseUrl);

        var clientResourceRequest = new ClientResourceRequest() { ResourceUri = "{id}", ResourceValueDictionary = new Dictionary<string, string>() { { "id", id } } };
        var restRequest = _httpClient.GetRequestWithResource(clientResourceRequest);

        var prodDetail = _httpClient.ExecuteRequestToGetDetails(restClient, restRequest) as ProductDetail;
        var response = new ProductDetailResponse();

        if (prodDetail == null) return response;
        response.Category = prodDetail.Category;
        response.Description = prodDetail.Description;
        response.EndDate = prodDetail.EndDate;
        response.Id = prodDetail.Id;
        response.Name = prodDetail.Name;
        response.Popularity = prodDetail.Popularity;
        response.StartDate = prodDetail.StartDate;
        return response;
    }
}

I have abstracted the calls to REST api in a wrapper like so

 public interface IClient
{
    object GetClient(string baseUrl);
    object GetRequest();
    object GetRequestWithResource(ClientResourceRequest resourceValueRequest);
    object ExecuteRequestToGetDetails(object client, object request);
    object ExecuteRequestToGetList(object client, object request);
}

public class ProductRestClient : IClient
{
    public object GetClient(string baseUrl)
    {
        return new RestClient(baseUrl);
    }

    public object GetRequest()
    {
        return new RestRequest(Method.GET);
    }

    public object GetRequestWithResource(ClientResourceRequest resourceValueRequest)
    {
        var request = new RestRequest(resourceValueRequest.ResourceUri, Method.GET);
        foreach (var item in resourceValueRequest.ResourceValueDictionary)
        {
            request.AddUrlSegment(item.Key, item.Value);
        }

        return request;
    }

    public object ExecuteRequestToGetDetails(object client, object request)
    {
        var restClient = client as RestClient;
        var restRequest = request as RestRequest;

        var result = restClient.Execute<ProductDetail>(restRequest);
        return result.Data;
    }

    public object ExecuteRequestToGetList(object client, object request)
    {
        var restClient = client as RestClient;
        var restRequest = request as RestRequest;

        var result = restClient.Execute<List<ProductBase>>(restRequest);
        return result.Data;
    }
}

The REST client wrapper request object look like so

 public class ClientResourceRequest
{
    public string ResourceUri{ get; set; }

    public Dictionary<string, string>  ResourceValueDictionary { get; set; }
}

the domain entitys are like so (used in restclient implementation and service but never made available to controllers)

 public class ProductBase : IEntityBase
{
    public DateTime EndDate { get; set; }
    public string Id { get; set; }
    public string Name { get; set; }
    public string Popularity { get; set; }
    public DateTime StartDate { get; set; }
}

public class ProductDetail : ProductBase, IEntityBase
{
    public string Category { get; set; }
    public string Description { get; set; }
    public string NumberOfPlays { get; set; }
}

the controllers are only aware of these request response objects (sort of viewmodels)

public class ProductListResponse
{
    public DateTime OnSaleDate { get; set; }
    public string Id { get; set; }
    public string Name { get; set; }
    public string Popularity { get; set; }
}

public class ProductDetailResponse
{
    public string Category { get; set; }
    public string Description { get; set; }
    public DateTime EndDate { get; set; }
    public string Id { get; set; }
    public string Name { get; set; }
    public string Popularity { get; set; }
    public DateTime StartDate { get; set; }
}

Any feedback on the quality and how extensible this code is, is much appreciated. Issues such as

1. if the endpoint from the API changes and starts using another parameter, how do I pre-empt for that change and code accordingly.

2. My rest client wrapper has domains (productbase/productdetails), anyway I can avoid using them?

3. Is it fine to return objects in my custom rest client calls? Should they be strongly typed? If yes how do i handle errors?

Plus if you could suggest a better way of dealing with these api calls to make them more performant, that would be great.

Answer 1

Few thoughts:

I personally very much dislike the IClient interface returning an object, which you then cast to a well known list of well known object. Makes no sense to me at all, what is the point?

The thing is, you actually know what fields are expected (because you strongly typed them) so what is the purpose of converting to and from object?

I think that what you should is either:

1. Stick to strongly typed object through the whole code. If the domain changes - i.e. your external api changes - you will need to update the whole application.
2. Stick with objects and only make cast (or even go with dynamics) in the view - in the example you given, there is actually not much business logic - just copying properties right to left, so why don't you just leave that in the view? In that case it may be a double win - the code is simpler and you can update views without reloading actual IIS application, comes very useful in production sometimes.
3. Last (and most far-fetched) option is to remove the whole server side client and make calls directly from JavaScript, but in this case you'd have to switch from server side MVC, probably to some client side MVC / MVVM like Knockout or AngularJS. It may not be suited to you at all if the REST server is isolated from external calls for example, or you have to pass some shared secret etc.

I personally would go with option 1 + eliminate all unnecessary models. If you don't modify the data from REST service, I don't see point in making separate models for view, specially that you did not use any formatting attributes etc. C# was designed to be used as strongly typed language, and besides very specific cases, we should stick to it.

Answer 2

Please don't call methods List() or Details(). Microsoft says: "Use verbs or verb phrases to name methods."

Note that GetList() also isn't a good name: what if you change your mind and decide to return an IEnumerable<T> instead of a List<T>?

---

ProductListResponse isn't a good name either, especially not when you return a List<ProductListResponse> -- a request should result in only one response, I'd hope. Isn't this a DTO anyway?

---

Why is the IProductService named quizService when it is a parameter?

public ProductController(IProductService quizService)
{
    _productService = quizService;
}

---

Try to reduce indentation in cases like this:

if (products != null)
{
    return products.Select(prod=> new ProductListResponse
    {
        Id = prod.Id,
        OnSaleDate = prod.EndDate,
        Name = prod.Name,
        Popularity = prod.Popularity
    }).ToList();
}
return new List<ProductListResponse>();

This could be rewritten as:

if (products == null)
{
    return new List<ProductListResponse>();
}

return products.Select(prod=> new ProductListResponse
{
    Id = prod.Id,
    OnSaleDate = prod.EndDate,
    Name = prod.Name,
    Popularity = prod.Popularity
}).ToList();

---

This just baffles me:

public object ExecuteRequestToGetDetails(object client, object request)
{
    var restClient = client as RestClient;
    var restRequest = request as RestRequest;

    var result = restClient.Execute<ProductDetail>(restRequest);
    return result.Data;
}

If you need a RestClient and a RestRequest, then why are you passing an object?

---

Perhaps you should rely more on libraries like MediatR or AutoMapper.