1
\$\begingroup\$

I have an asynchronous controller. It is supposed to make a couple async calls then return combined data from then as one DTO.

[HttpGet("[action]/{name}")]
public async Task<CombinedDTO> GetSummonerData(string name)
{
    SummonerEndPoint endPoint = new SummonerEndPoint();
    SummonerDTO summonerData=  await endPoint.GetSummonerData(name);
    MatchesEndPoint endPoint2 = new MatchesEndPoint();
    MatchListDto matchesData =  await endPoint2.GetMatches(summonerData.accountId.ToString());
    return await TestMethod(summonerData, matchesData);
}
public async Task<CombinedDTO> TestMethod(SummonerDTO summonerData, MatchListDto matchData)
    {
         return await Task.Run(() => {
               return new CombinedDTO {
                    summoner = summonerData,
                    matches = matchData }; });
    }

Code above seems to be working but I am not sure is this is the best way to combining two DTOs into one.

for context endpoints:

public class SummonerEndPoint : BaseEndPoint
{ 
    public SummonerEndPoint() => Url = @"https://eun1.api.riotgames.com/lol/summoner/v3/summoners/by-name/";

    public async Task<SummonerDTO> GetSummonerData(string summonerName)
    {
        var data = await client.MakeAsyncRequest(url + summonerName).ConfigureAwait(false);
        return JsonConvert.DeserializeObject<SummonerDTO>(data);
    }
}

public class MatchesEndPoint : BaseEndPoint
    {
        public MatchesEndPoint() => Url = @"https://eun1.api.riotgames.com/lol/match/v3/matchlists/by-account/";
        public async Task<MatchListDto> GetMatches(string accountId)
        {
            var data = await client.MakeAsyncRequest(Url + accountId + "?beginIndex=0&endIndex=10").ConfigureAwait(false);
            return JsonConvert.DeserializeObject<MatchListDto>(data);
        }
    }

public class BaseEndPoint
    {
        protected string url;
        protected string Url { get => url; set => url = value; }
        protected RiotApiRequest client = new RiotApiRequest();
    }

public class RiotApiRequest : RiotApiRequestBase
    {
        public async Task<string> MakeAsyncRequest(string url)
        {
            using (var response = await MakeRequest(url).ConfigureAwait(false))
            {
                return await GetResponseContentAsync(response).ConfigureAwait(false);
            }
        }

        protected async Task<string> GetResponseContentAsync(HttpResponseMessage response)
        {
            using (response)
            using (var content = response.Content)
            {
                return await content.ReadAsStringAsync().ConfigureAwait(false);
            }
        }
    }

public class RiotApiRequestBase
    {
        protected async Task<HttpResponseMessage> MakeRequest(string url)
        {
            using( HttpClient client = new HttpClient())
            {
                //later this may be moved elsewhere ;)
                client.DefaultRequestHeaders.Add("X-Riot-Token", "Token");
                return await client.GetAsync(url).ConfigureAwait(false);

            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Could you provide example DTO data or the code from SummonerEndPoint and MatchesEndPoint for context? \$\endgroup\$ – Phrancis Jun 21 '18 at 4:42
  • \$\begingroup\$ @Phrancis i modified question with ednpoints \$\endgroup\$ – szpic Jun 21 '18 at 5:15
  • \$\begingroup\$ Excellent, thank you! I hope you get great answers. \$\endgroup\$ – Phrancis Jun 21 '18 at 5:30
  • \$\begingroup\$ @szpic Short answer would be yes. However you have a few other issues with the code shown. Is this asp.net-core? \$\endgroup\$ – Nkosi Jun 21 '18 at 14:41
  • \$\begingroup\$ @Nkosi yes it is. I would be gratefull for list of those issues! \$\endgroup\$ – szpic Jun 21 '18 at 16:26
3
\$\begingroup\$

Short answer would be yes. However you have a few other issues with the code shown.

There is no need for the Task.Run in the example TestMethod.

public async Task<CombinedDTO> TestMethod(SummonerDTO summonerData, MatchListDto matchData) {
    return await Task.Run(() => {
        return new CombinedDTO {
            summoner = summonerData,
            matches = matchData 
        };
    });
}

For a simple object like your model there really is no need to throw that on to a new thread. The DTO can be constructed inline.

[HttpGet("[action]/{name}")]
public async Task<CombinedDTO> GetSummonerData(string name) {
    SummonerEndPoint endPoint = new SummonerEndPoint();
    SummonerDTO summonerData =  await endPoint.GetSummonerData(name);
    MatchesEndPoint endPoint2 = new MatchesEndPoint();
    MatchListDto matchesData =  await endPoint2.GetMatches(summonerData.accountId.ToString());
    return new CombinedDTO {
        summoner = summonerData,
        matches = matchData 
    };
}

With that out of the way, this code is ripe for refactoring.

Starting from the bottom RiotApiRequestBase should not constantly create and dispose of HttpClient as this can have adverse effects (socket exhaustion).

public interface IRiotApiClient {
    Task<HttpResponseMessage> GetAsync(string url);
}

public class RiotApiClient : IRiotApiClient {
    static Lazy<HttpClient> client = new Lazy<HttpClient>(() => {
        //later this may be moved elsewhere 
        var client = new HttpClient();
        client.DefaultRequestHeaders.Add("X-Riot-Token", "Token");
        return client;
    });

    protected Task<HttpResponseMessage> GetAsync(string url) {
        return client.Value.GetAsync(url);
    }
}

Keep an instance for the lifetime of the application. Alternatively, if you are using a more recent version of .net core consider using IHttpClientFactory.

Note the abstraction of the client, it will come into play later on.

The response in MakeAsyncRequest is already going to be disposed by its using. No need to wrap it again in GetResponseContentAsync. This also breaks Single Responsibility Principle as GetResponseContentAsync has no business disposing of the response.

Based on the abstraction of the client, that functionality can be refactored to extension methods.

public static class RiotApiClientExtensions {

    public static async Task<TResult> GetAsAsync<TResult>(this IRiotApiClient client, string url) {
        var data = await client.GetStringAsync(url);
        return JsonConvert.DeserializeObject<TResult>(data);
    }

    public static async Task<string> GetStringAsync(this IRiotApiClient client, string url) {
        using (var response = await client.GetAsync(url).ConfigureAwait(false)) {
            return await ReadContentAsStringAsync(response).ConfigureAwait(false);
        }
    }

    protected static async Task<string> ReadContentAsStringAsync(HttpResponseMessage response) {
        using (var content = response.Content) {
            return await content.ReadAsStringAsync().ConfigureAwait(false);
        }
    }
}

You appear to be following a format similar to CQRS (Command Query Responsibility Segregation), with your end points acting as queries.

I've used the following abstractions to help standardize queries.

using System.Threading.Tasks;

public interface IAsyncFunction<TResponse> {
    Task<TResponse> ExecuteAsync();
}

public interface IAsyncFunction<T, TResponse> {
    Task<TResponse> ExecuteAsync(T parameter);
}

public interface IAsyncFunction<T1, T2, TResponse> {
    Task<TResponse> ExecuteAsync(T1 parameter1, T2 parameter2);
}

public interface IAsyncFunction<T1, T2, T3, TResponse> {
    Task<TResponse> ExecuteAsync(T1 parameter1, T2 parameter2, T3 parameter3);
}

A similar format could be used for Actions/Commands

Using the above, look at the transformation of SummonerEndPoint and MatchesEndPoint

public interface ISummonerEndPoint : IAsyncFunction<string, SummonerDTO> { }

public class SummonerEndPoint : ISummonerEndPoint {
    private readonly string baseUrl;
    private readonly IRiotApiClient client;

    public SummonerEndPoint(IRiotApiClient client) {
        this.client = client;
        baseUrl = @"https://eun1.api.riotgames.com/lol/summoner/v3/summoners/by-name/";
    }

    public Task<SummonerDTO> ExecuteAsync(string summonerName) {
        return client.GetAsAsync<SummonerDTO>(baseUrl + summonerName);
    }
}

public interface IMatchesEndPoint : IAsyncFunction<string, MatchListDto> { }

public class MatchesEndPoint : IMatchesEndPoint {
    private readonly string baseUrl;
    private readonly IRiotApiClient client;

    public MatchesEndPoint(IRiotApiClient client) {
        this.client = client;
        baseUrl = @"https://eun1.api.riotgames.com/lol/match/v3/matchlists/by-account/";
    }
    public Task<MatchListDto> ExecuteAsync(string accountId) {
        return client.GetAsAsync<MatchListDto>(baseUrl + accountId + "?beginIndex=0&endIndex=10");
    }
}

Controller actions should be kept as lean as possible. It should really have only one purpose (SRP).

The initialization of the end points can now be inverted out of the controller and explicitly injected. This reduces the responsibilities of the controller and simplifies the action.

[Route("[controller]")]
public class MyController : Controller {
    private readonly IMatchesEndPoint getMatches;
    private readonly ISummonerEndPoint getSummonerData;

    public MyController(ISummonerEndPoint getSummonerData, IMatchesEndPoint getMatches) {
        this.getMatches = getMatches;
        this.getSummonerData = getSummonerData;
    }

    [HttpGet("[action]/{name}")]
    public async Task<IActionResult> GetSummonerData(string name) {
        SummonerDTO summonerData =  await getSummonerData.ExecuteAsync(name);
        MatchListDto matchesData =  await getMatches.ExecuteAsync(summonerData.accountId.ToString());
        var combinedDto = new {
            summoner = summonerData,
            matches = matchData 
        };
        return Ok(combinedDto);
    }
}

Ideally the controller could have been simplified even more with a service that encapsulates the combining of the DTO for reuse but that is totally up to you.

With that all abstractions and their implementations should be registered at the composition root on startup

\$\endgroup\$

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.