Call WebAPI then parse part of json result

Question

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": [
      {
        ...
      },
      {
        ...
      }
    ],
  }
}

Answer 1

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();
}

Answer 2

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().