2
\$\begingroup\$

Our Project architecture is AngularJS + MVC + WebAPI. Angular JS would call MVC action and MVC action further calls API methods. Below are some of the action methods we used for calling api methods. Note that our project is in its final stage, and as such we cannot modify the existing architecture. We have to use AngularJS + MVC + WebAPI.

We are having lot of controllers and each controller contains lot of get/post methods. I need suggestion about below part of code. because similar to below piece of code only we are using across all the controllers with different method names. Is there any other better way/ Generic way to call webapi from MVC.

    public async Task<ActionResult> GetStates()
                {

                    httpClient = HttpContext.Items["httpclient"] as HttpClient;
                    HttpResponseMessage response = await httpClient.GetAsync(httpClient.BaseAddress + "api/Admin/GetStates");
                    if (response.IsSuccessStatusCode)
                    {
                        string stateInfo = response.Content.ReadAsStringAsync().Result;
                        if (!string.IsNullOrEmpty(stateInfo))
                        {
                            return new ContentResult { Content = stateInfo, ContentType = "application/json" };
                        }
                    }

                    return new JsonResult();
                }
                public async Task<ActionResult> ClientsInfo()
                {

                    httpClient = HttpContext.Items["httpclient"] as HttpClient;
                    HttpResponseMessage response = await httpClient.GetAsync(httpClient.BaseAddress + "api/Admin/GetClients");
                    if (response.IsSuccessStatusCode)
                    {
                        string roleInfo = response.Content.ReadAsStringAsync().Result;
                        if (!string.IsNullOrEmpty(roleInfo))
                        {
                            return new ContentResult { Content = roleInfo, ContentType = "application/json" };
                        }
                    }

                    return new JsonResult();
                }

                public async Task<ActionResult> GetRoles()
                {
                    httpClient = HttpContext.Items["httpclient"] as HttpClient;
                    HttpResponseMessage response = await httpClient.GetAsync(httpClient.BaseAddress + "api/Roles");
                    if (response.IsSuccessStatusCode)
                    {
                        string roleInfo = response.Content.ReadAsStringAsync().Result;
                        if (!string.IsNullOrEmpty(roleInfo))
                        {
                            return new ContentResult { Content = roleInfo, ContentType = "application/json" };
                        }
                    }

                    return new JsonResult();
                }
...........
//for post and delete actions similarly
\$\endgroup\$
2
\$\begingroup\$

I agree as some others have said that in this case, having a layer between Angular and your API is simply bloat / overhead unless there are specific reasons for the extra abstraction. I understand that you are late in the project development so my review will reflect that but as an aside I have to point out that your management should have a COMPLETE understanding of the technical debt incurred by any potential "Escalation of Commitment" mistakes made by them. Especially considering Web API will eventually be deprecated: https://blog.tonysneed.com/2016/01/06/wcf-is-dead-long-live-mvc-6/

That being said, your code for the most part is fine but I would suggest a couple of architectural changes.

1. Keep your controllers thin

Controllers only responsibility should be to retrieve data and pass that data to the view. In this case, your view is JSON, so your controller should ONLY make a cal to get data.

As the code currently is it is also more difficult to test than it needs to be.

2. Encapsulate methods

As you have pointed out, you are winding up with a lot of duplicate code. I would suggest solving both problems by introducing another layer of indirection.

Example Code

Controller Code

public class MyController : Controller {
    private IApiClient _apiClient;

    public MyController() : this(new WebApiClient()) {
        // Poor-man's DI in case you don't have an IoC container.
        // If you do, do not include this constructor
    }

    public MyController(IApiClient apiClient) : base() {
        this._apiClient = apiClient; // assign injected dependency
        // This injected dependency streamlines your testing process
        // because the results are more easily mocked than direct access
        // to the HttpContext as was done in the actions before
    }

    public async Task<ActionResult> GetStates() {
        string relativePath = "api/Admin/GetStates";
        return await this.ApiGet(relativePath);
    }

    public async Task<ActionResult> GetClients() {
        string relativePath = "api/Admin/GetClients";
        return await this.ApiGet(relativePath);
    }

    public async Task<ActionResult> Roles() {
        string relativePath = "api/Roles";
        return await this.ApiGet(relativePath);
    }

    protected async Task<ActionResult> ApiGet(string relativePath) {
        ApiResponse response = await this.ApiClient.GetAsync(relativePath);
        ActionResult result = response.IsSuccessful ? new ContentResult { Content = response.Data, ContentType = "application/json" } as ActionResult : new JsonResult();
        return result;
    }

    protected IApiClient ApiClient {
        get {
            return this._apiClient;
        }
    }
}

The ApiResponse class

[Serializable]
public class ApiResponse {
    private bool _successful;
    private string _data;

    public ApiResponse() {
        this._data = string.Empty;
        this._successful = false;
    }

    public string Data {
        get {
            return this._data;
        } set {
            this._data = value;
        }
    }

    public bool IsSuccessful {
        get {
            return this._successful;
        } set {
            this._successful = value;
        }
    }
}

The IApiClient and WebApiClient types

public interface IApiClient {
    Task<ApiResponse> GetAsync(string relativePath);
}

public class WebApiClient : IApiClient {
    private System.Net.Http.HttpClient _httpClient;

    public WebApiClient() {
    }

    protected Uri BuildCompleteUri(string relativePath) {
        // Using a URI will help us to validate the URL before making an invalid call.
        Uri returnUri = new Uri(VirtualPathUtility.Combine(this.Client.BaseAddress.ToString(), relativePath));

        return returnUri;
    }

    public async Task<ApiResponse> GetAsync(string relativePath) {
        Uri completeUri = this.BuildCompleteUri(relativePath);
        ApiResponse response = new ApiResponse();
        HttpResponseMessage apiResponse = await this.Client.GetAsync(completeUri);
        if (apiResponse.IsSuccessStatusCode) {
            string responseData = apiResponse.Content.ReadAsStringAsync().Result;
            if (!string.IsNullOrWhiteSpace(responseData)) {
                response.IsSuccessful = true;
                response.Data = responseData;
            }
        } else {
            // TODO: Log Errors
        }

        return response;
    }

    public HttpClient Client {
        get {
            if (this._httpClient == null) {
                this._httpClient = HttpContext.Current.Items["httpclient"] as HttpClient;
            }

            return this._httpClient;
        }
    }
}

The above code structure keeps your controllers thin, reduces code duplication, allows for extension points if you need to add more information of your own to the ApiResponse type, and dramatically simplifies your unit testing process because the IApiClient type is easily mocked.

You could even take it a step further and use the DI container to register a factory method that injects the HttpClient instance into the WebApiClient type, which would also simplify the testing process of the WebApiClient.

I also renamed your controller actions so they are consistent with the API method they invoke.

\$\endgroup\$
  • \$\begingroup\$ You posted the link about asp 5 just in time, I was going to use web api for the next project ;-) \$\endgroup\$ – t3chb0t Aug 9 '16 at 14:33
  • \$\begingroup\$ @t3chb0t Glad to help. :) I was surprised when I found out as well, but I suppose it makes sense. \$\endgroup\$ – xDaevax Aug 9 '16 at 15:05
  • \$\begingroup\$ another option would be to implement ApiGet method at BaseController level & let all other controllers inherit BaseController in order make this more modular & make ApiGet method available to all controllers. \$\endgroup\$ – Mads... Aug 10 '16 at 9:41
  • 1
    \$\begingroup\$ @Mady Good point. In my example, I made the assumption that not all controllers for the OP's question make API calls so I didn't provide a base implementation but a separate base-type for controllers that make API calls could be created with that method to further reduce controller code. \$\endgroup\$ – xDaevax Aug 10 '16 at 12:55
1
\$\begingroup\$

The code described above could employ a generic method which could take all the common and repetitive code and just take in the endpoint (e.g. "api/Roles") as a parameter. This could help to reduce the code down and reduce the repetitiveness.

Here's a general method implementation example:

    public async Task<ActionResult> ClientsInfo()
    {
        return await ProcessRequest("api/Admin/GetClients");
    }


    public async Task<ActionResult> ProcessRequest(string endpoint)
    {
        var httpClient = HttpContext.Items["httpclient"] as HttpClient;

        var response = await httpClient.GetAsync(httpClient.BaseAddress + endpoint);

        if (response.IsSuccessStatusCode)
        {
            string roleInfo = response.Content.ReadAsStringAsync().Result;
            if (!string.IsNullOrEmpty(roleInfo))
            {
                return new ContentResult { Content = roleInfo, ContentType = "application/json" };
            }
        }

        return new JsonResult();
    }

As you mentioned, I understand that you cannot change the architecture, but for the record, the MVC layer is not actually doing anything useful, so it could be removed. WebApi can do authorization as well, it's built in (on top of MVC). I would review this related link and remove the MVC layer from this.

Simplifying the code base will make it easier to implement and maintain in the future.

\$\endgroup\$
  • \$\begingroup\$ I understand your sentiment, but for future compatibility, I would actually remove the Web API layer (since in .NET Core MVC, Web API is condensed down into MVC and will no longer be a separate framework). See the article here: blog.tonysneed.com/2016/01/06/wcf-is-dead-long-live-mvc-6 \$\endgroup\$ – xDaevax Aug 9 '16 at 13:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.