4
\$\begingroup\$

this is a basic web api call using HttpClient and parse some part of the result, this was working as expected, but the part inside the if (response.IsSuccessStatusCode) doesnt seems right. It feels like it can be simplified, but I don't know how.

static async Task<List<Hero>> GetHero(string url, string urlParameters)
{
    HttpClient client = new HttpClient();
    client.BaseAddress = new Uri(url);
    client.DefaultRequestHeaders.Accept.Clear();
    client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

    using (var response = client.GetAsync(urlParameters).Result)
    {
        if (response.IsSuccessStatusCode)
        {
            var heroes = new List<Hero>();
            var jsonResult = await response.Content.ReadAsStringAsync();
            var deserializedjsonResult = JObject.Parse(jsonResult);
            var resultHeroes = deserializedjsonResult["result"]["heroes"].ToList();

            resultHeroes.ForEach(x => heroes.Add(x.ToObject<Hero>()));

            return heroes;
        }
    }

    return null;
}

the api that I was calling will return something in this format

{
  "result": {
    "heroes": [
      {
        ...
      },
      {
        ...
      }
    ],
    "villains": [
      {
        ...
      },
      {
        ...
      }
    ],
    "extras": [
      {
        ...
      },
      {
        ...
      }
    ],
  }
}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$
HttpClient client = new HttpClient();

This needs to be disposed.


using (var response = client.GetAsync(urlParameters).Result)

Why don't you await this like you do with other awaitable APIs?


static async Task<List<Hero>> GetHero(string url, string urlParameters)
{
    ..
    return null;
}

Methods returning collections should return an empty collection instead of null this means you should

return Task.FromResult(new List<Hero>());

As far as the if block is concerned you don't need the heros variable. With LINQ can create the list inplace:

if (response.IsSuccessStatusCode)
{
    var jsonResult = await response.Content.ReadAsStringAsync();
    var deserializedjsonResult = JObject.Parse(jsonResult);
    var resultHeroes = deserializedjsonResult["result"]["heroes"];
    return resultHeroes.Select(x => x.ToObject<Hero>()).ToList();
}
\$\endgroup\$
8
  • \$\begingroup\$ thanks, on the await on client.GetAsync, Im getting error, 'HttpResponseMessage' does not contain a definition for 'GetAwaiter' and no extension method 'GetAwaiter' , any commennts on how I parse the result and put it to c# object? \$\endgroup\$
    – user140692
    Jun 8, 2017 at 7:21
  • \$\begingroup\$ @user140692 you should not await the Result but the GetAsync \$\endgroup\$
    – t3chb0t
    Jun 8, 2017 at 7:26
  • \$\begingroup\$ thanks, im still getting used to the async/task. any idea about the part inside if (response.IsSuccessStatusCode)? \$\endgroup\$
    – user140692
    Jun 8, 2017 at 7:30
  • \$\begingroup\$ WRT HttpClient needing to be disposed: stackoverflow.com/a/15708633/648075 \$\endgroup\$
    – BCdotWEB
    Jun 8, 2017 at 11:35
  • 1
    \$\begingroup\$ Then may I suggest IList<T> as an alternative to adhere to developing to interfaces? \$\endgroup\$ Jun 8, 2017 at 18:19
0
\$\begingroup\$

I have a few minor and some major suggestions.

static async Task<List<Hero>> GetHero(string url, string urlParameters)

Missing public/private. You are using a concrete type here, and you should be using an interface. The usual practice in .NET here is to take the "lowest common denominator", which in this case is IEnumerable<Hero> or slightly higher up is ICollection<Hero>. This way you can internally change the return type as long as it implements one of those interfaces. This does not prevent you using .ToList() because List<T> implements IEnumerable<T>.

See here for more: https://stackoverflow.com/questions/10113244/why-use-icollection-and-not-ienumerable-or-listt-on-many-many-one-many-relatio

Basically, use an interface. I would also strongly discourage IList<T> unless you really want to force the consumer to deal with lists. If you don't need the extra methods that IList<T>, then use the lower interface, and so on.

ForEach

The LINQ ForEach method is often regarded as a bad idea due to it causing multiple enumerations, and in fact one of the .NET developers themselves suggests not using it: https://blogs.msdn.microsoft.com/ericlippert/2009/05/18/foreach-vs-foreach/

In your case, it can be rewritten to using Select().

\$\endgroup\$
1
  • \$\begingroup\$ I'm not sure about the last point. The article is about a LINQ ForEach extension but OP is using the List<>.ForEach API. It is also not suggesting to not use it but explains why there is no such extension. What do you mean by causing multiple enumerations? \$\endgroup\$
    – t3chb0t
    Jun 9, 2017 at 14:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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