Async wrapper around public API

Question

Tear it apart. I'm mostly concerned around the appropriate use of ConfigureAwait(false), and possible poor handling and duplication of the HttpClient in Invest and Get<T>.

Additionally, I'd also like thoughts on the fact that for usage of these methods, one must call .Result to get the actual results. Is this standard for building such a wrapper or is there another way I should achieve this?

using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Http;
using System.Net.Http.Headers;
using System.ServiceModel;
using System.Text;
using System.Threading.Tasks;
using Newtonsoft.Json;
namespace ProsperAPI
{
 public class ProsperApi
 {
 private readonly string _username;
 private readonly string _password;
 private readonly string _apiBaseUrl = "https://api.prosper.com/v1/";
 private AuthenticationHeaderValue _authenticationHeader;
 #region Constructors
 public ProsperApi(string username, string password)
 {
 _username = username;
 _password = password;
 _authenticationHeader = 
 new AuthenticationHeaderValue(
 "Basic",
 Convert.ToBase64String(
 Encoding.UTF8.GetBytes(
 string.Format("{0}:{1}", _username, _password))));
 }
 public ProsperApi(string username, string password, string baseUrl) : this(username, password)
 {
 _apiBaseUrl = baseUrl;
 }
 #endregion
 public async Task<bool> Authenticate()
 {
 if (String.IsNullOrEmpty(_username))
 throw new ArgumentNullException("_username", "Credentials are not set");
 if (String.IsNullOrEmpty(_username))
 throw new ArgumentNullException("_password", "Credentials are not set");
 try
 {
 // The account call will fail out if credentials are incorrect, thus
 // we won't spend time getting Notes. If Account information is right,
 // then we load the notes data at the same time, so we can use it 
 await GetAccount().ConfigureAwait(false);
 return true;
 }
 catch (Exception)
 {
 return false;
 }
 }
 public async Task<List<Note>> GetNotes()
 {
 return await Get<List<Note>>("notes/").ConfigureAwait(false);
 }
 public async Task<Account> GetAccount()
 {
 return await Get<Account>("account/").ConfigureAwait(false);
 }
 public async Task<List<Listing>> GetListings()
 {
 return await Get<List<Listing>>("Listings/").ConfigureAwait(false);
 }
 public async Task<List<Investment>> GetPendingInvestments()
 {
 return await Get<List<Investment>>("Investments/$filter=ListingStatus eq 2").ConfigureAwait(false);
 }
 public async Task<InvestResponse> Invest(string listingId, string amount)
 {
 using (var client = HttpClientSetup())
 {
 var investment = new List<KeyValuePair<string, string>>
 {
 new KeyValuePair<string, string>("listingId", listingId),
 new KeyValuePair<string, string>("amount", amount)
 };
 var content = new FormUrlEncodedContent(investment);
 var response = await client.PostAsync("Invest/", content).ConfigureAwait(false);
 if (response.StatusCode != HttpStatusCode.OK)
 throw new CommunicationException();
 var obj = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
 return JsonConvert.DeserializeObject<InvestResponse>(obj);
 }
 }
 private async Task<T> Get<T>(string url)
 {
 using (var client = HttpClientSetup())
 {
 var response = await client.GetAsync(url).ConfigureAwait(false);
 if (response.StatusCode != HttpStatusCode.OK)
 throw new CommunicationException();
 var obj = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
 return JsonConvert.DeserializeObject<T>(obj);
 }
 }
 private HttpClient HttpClientSetup()
 {
 var client = new HttpClient {BaseAddress = new Uri(_apiBaseUrl)};
 client.DefaultRequestHeaders.Accept.Clear();
 client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
 client.DefaultRequestHeaders.Authorization = _authenticationHeader;
 return client;
 }
 }
}

• Mathieu Guindon: Welcome to Code Review! Nice first post, "tear it apart" ... lol, that's the spirit! :)

Answer 1

There's something odd about your constructors - normally chained constructors call the constructor with the most parameters, not the other way around:

public ProsperApi(string username, string password, string baseUrl) 
 : this(username, password)

public ProsperApi(string username, string password)

Should be:

public ProsperApi(string username, string password)
 : this(username, password, null)

public ProsperApi(string username, string password, string baseUrl) 

_apiBaseUrl will be null anyway when you call the 2-parameter constructor. The idea is that you write - and maintain, one constructor body.

This would be it:

public ProsperApi(string username, string password)
 : this(username, password, null)
{ }
public ProsperApi(string username, string password, string baseUrl) 
{
 _username = username;
 _password = password;
 _apiBaseUrl = baseUrl;
 _authenticationHeader = 
 new AuthenticationHeaderValue(
 "Basic",
 Convert.ToBase64String(
 Encoding.UTF8.GetBytes(
 string.Format("{0}:{1}", _username, _password))));
}

With only 1 constructor doing all the construction work, you don't really need a #region here; regions are seldom ever really needed anyway, and more often than not their usage is a smell: if you need to "sectionize" a class, it's likely doing too many things. Now a Constructors region is different, it's more like a comment that says what the code already say.

Besides, you can collapse the constructor bodies, leaving only the signatures visible in the IDE; that's possibly more descriptive than any comment you can write there.

---

I like your private fields, private readonly fields assigned in the constructor are great. Why isn't _authenticationHeader readonly as well? If it's only meant to be assigned by the constructor, and not tampered with, then it should be made readonly.

---

In the Authenticate method, your guard clause is a bit weird:

if (String.IsNullOrEmpty(_username))
 throw new ArgumentNullException("_username", "Credentials are not set");
if (String.IsNullOrEmpty(_username))
 throw new ArgumentNullException("_password", "Credentials are not set");

You're not validating the _password (you're checking for a null or empty _username write) - nice copy+paste error you got here. But nonetheless, the goal of a guard clause is to fail early, and with _username and _password being readonly and only assignable in the constructor, wouldn't it make more sense to throw at construction time, if you're going to pass illegal constructor arguments that will make the object fail later, might as well throw right away and refuse to create an instance with null/empty credentials.

Other than by the method's name, it's not very clear why Authenticate needs _username and _password to be set, one has to trace all the way down to HttpClientSetup() to see _authenticationHeader in use, with the actual method call that would probably fail if _username or _password was null or empty. Preventing null/empty credentials at construction would ensure that the instance is always in a state that allows it to make these calls without worrying about the username and password values.

You're storing username and password as private readonly fields, but in reality you don't need to keep either - you only need them to construct the _authenticationHeader. I wouldn't keep that data in the instance, it's not needed. I'd change your constructor to this:

public ProsperApi(string username, string password, string baseUrl) 
{
 if (string.IsNullOrWhiteSpace(username) || string.IsNullOrWhiteSpace(password))
 {
 throw new ArgumentException("Username and password cannot be null or empty.");
 }
 _apiBaseUrl = baseUrl;
 _authenticationHeader = 
 new AuthenticationHeaderValue(
 "Basic",
 Convert.ToBase64String(
 Encoding.UTF8.GetBytes(
 string.Format("{0}:{1}", username, password))));
}

Using string.IsNullOrWhiteSpace over string.IsNullOrEmpty will also refuse to take " " as a valid user name or password.

I went with an ArgumentException, because ArgumentNullException should be specifically for when an argument is null when it shouldn't be - and in this case the argument being null isn't the only way to trip the guard clause, so I'd rather throw a more general exception than a potentially lying/confusing specific exception - there doesn't need to be two separate checks, since the exception message will be the same either way.

---

Lastly, I like how you've named [almost] everything, except the async methods should, by convention, be appended with the word Async as a suffix, like:

public async Task<List<Note>> GetNotesAsync()
public async Task<Account> GetAccountAsync()
public async Task<List<Listing>> GetListingsAsync()
private async Task<T> GetAsync<T>(string url)

Not sure Get<T> or GetAsync<T> is a very descriptive name though. Perhaps GetJsonResultAsync<TResult>?

Answer 2

I'm mostly concerned around the appropriate use of ConfigureAwait(false) ...

Your usage is correct. In library code like this, every await should use ConfigureAwait(false).

Though one thing I would do differently are the methods that call Get. Instead of:

public async Task<List<Note>> GetNotes()
{
 return await Get<List<Note>>("notes/").ConfigureAwait(false);
}

You can write just:

public Task<List<Note>> GetNotes()
{
 return Get<List<Note>>("notes/");
}

---

... and possible poor handling and duplication of the HttpClient in Invest and Get<T>.

You could reduce the duplication by creating a helper function like:

private async Task<T> GetResponse<T>(
 Func<HttpClient, Task<HttpResponseMessage>> createRequestFunc)

Both Get and Invest would call this method, with the different parts passed in as a lambda.

---

var investment = new List<KeyValuePair<string, string>>
{
 new KeyValuePair<string, string>("listingId", listingId),
 new KeyValuePair<string, string>("amount", amount)
};
var content = new FormUrlEncodedContent(investment);

Here, investment has to be IEnumerable<KeyValuePair<string, string>>. Dictionary<string, string> also implements that interface and since order doesn't matter here and it would make the code simpler, I would use that:

var investment = new Dictionary<string, string>
{
 { "listingId", listingId },
 { "amount", amount }
};

---

Additionally, I'd also like thoughts on the fact that for usage of these methods, one must call .Result to get the actual results.

No! To use these methods, one should use await. If you're going to use Result:

1. You didn't have to bother with async, it won't given you any advantage, since Result will block a thread.
2. You are likely to get a deadlock if you forgot ConfigureAwait(false) somewhere.

• Prescott: Can you explain your simplification of removing the async and ConfigureAwait(false). Why are they unnecessary?
• svick: @Prescott Because that Task that's returned by Get already behaves exactly the way you want. So you don't need to create another Task that awaits the original one, you can just return the original directly.