3
\$\begingroup\$

I have a Send method that is working but want to refactor a portion where it is using PostAsync().Result and ReadAsStringAsync().Result. I've never written async methods before and don't feel I understand the core concept or pattern yet. Here is the original method:

public ServiceResult<string> Send(string tabDelimitedPOs)
{
    var result = new ServiceResult<string>();

    ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12;

    httpClient.DefaultRequestHeaders.ExpectContinue = false;

    var byteArray = Encoding.ASCII.GetBytes($"{User}:{Secret}");

    httpClient.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Basic", Convert.ToBase64String(byteArray));
            
    HttpResponseMessage response = null;

    HttpContent httpContent = new StringContent(tabDelimitedPOs);
    httpContent.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("text/plain");

    response = httpClient.PostAsync(URL, httpContent).Result;

    httpContent.Dispose();

    result.StatusCode = response.StatusCode;

    if (response.StatusCode == HttpStatusCode.OK || response.StatusCode == HttpStatusCode.Created)
    {
        result.IsFaulted = false;
        result.ReturnedObject = response.Content.ReadAsStringAsync().Result;
    }
    else
    {
        result.IsFaulted = true;
        result.FaultMessage = response.ReasonPhrase;
    }

    return result;
}

Would you critique the following refactor attempt? I've added two asynchronous methods.

public ServiceResult<string> Send(string tabDelimitedPOs)
{
    var result = new ServiceResult<string>();

    ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12;

    httpClient.DefaultRequestHeaders.ExpectContinue = false;

    var byteArray = Encoding.ASCII.GetBytes($"{User}:{Secret}");

    httpClient.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Basic", Convert.ToBase64String(byteArray));
        
    HttpContent httpContent = new StringContent(tabDelimitedPOs);
    httpContent.Headers.ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("text/plain");

    Task<HttpResponseMessage> taskResponse = PostData(URL, httpContent);
        
    httpContent.Dispose();

    result.StatusCode = taskResponse.Result.StatusCode;

    if (taskResponse.Result.StatusCode == HttpStatusCode.OK || taskResponse.Result.StatusCode == HttpStatusCode.Created)
    {
        result.IsFaulted = false;
        Task<string> taskString = ReadResponse(taskResponse.Result);
        result.ReturnedObject = taskString.Result;
    }
    else
    {
        result.IsFaulted = true;
        result.FaultMessage = taskResponse.Result.ReasonPhrase;
    }

    return result;
}

private async Task<HttpResponseMessage> PostData(string url, HttpContent content)
{
    HttpResponseMessage response = await httpClient.PostAsync(url, content);
    return response;
}

private async Task<string> ReadResponse(HttpResponseMessage response)
{
    string responseString = await response.Content.ReadAsStringAsync();
    return responseString;
}

Here is the rest of the class. This project is .NET Framework 4.5.2. The application is designed to run in a Server 2008 environment which I do not manage.

public class YoozAPI
{
    public string URL { get; set; }
    public string User { get; set; }
    public string Secret { get; set; }
    private static HttpClient httpClient = new HttpClient();

    public YoozAPI() { }

    public static YoozAPI Initialize()
    {
        string jsonFile = Path.Combine(Environment.CurrentDirectory, "YoozAPI.json");

        StreamReader reader = new StreamReader(jsonFile);
        string json = reader.ReadToEnd();
        return JsonConvert.DeserializeObject<YoozAPI>(json);
    }

    public bool PostOrders(PurchaseOrders orders)
    {
        string tabDelimitedPOs = orders.GetTabDelimitedString();
        ServiceResult<string> result = Send(tabDelimitedPOs);

        if (result.IsFaulted)
            Log.RecordToDatabase("ExportReleasedPOs", string.Empty, string.Empty, string.Empty, string.Empty, result.FaultMessage);

        return !result.IsFaulted;
    }

    // Code you've already seen above
}
\$\endgroup\$
4
  • \$\begingroup\$ What is .NET version? Also read this article and about Asynchronous programming. \$\endgroup\$
    – aepot
    Jun 30, 2021 at 21:01
  • \$\begingroup\$ One more tip: SecurityProtocol = SecurityProtocolType.Tls12 has no sense in .NET Framework 4.6 or newer, and has no effect (doesn't work for HttpClient) in .NET Core 3 or newer. \$\endgroup\$
    – aepot
    Jun 30, 2021 at 21:13
  • \$\begingroup\$ The version is .NET Framework 4.5.2 \$\endgroup\$ Jun 30, 2021 at 21:19
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Jul 1, 2021 at 7:10

1 Answer 1

3
\$\begingroup\$

Welcome to Code Review.

There's a lot of points that I may describe here, then I'll do it short.

Architecture

The wrong point here is sequence of instatiation settings for API helper class and the class itself. Originally instance created first, and only then the proberties are filled. It makes difficult to properly setup HttpClient.

I suggest to split settings and helper to reverse the instantiation order.

public class YoozApiSettings
{
    public string Url { get; set; }
    public string User { get; set; }
    public string Secret { get; set; }
}

Naming guidline: don't use uppercased names for names of more than 2 characters long. For example IO is fine but URL isn't. Renaming URL to Url. The same for the class names.

Reading file

Here's a first occurance of a wrong IDisposable usage. You open the file YoozAPI.json but never close it. Let's fix.

Original

StreamReader reader = new StreamReader(jsonFile);
string json = reader.ReadToEnd();

You may add reader.Close() or reader.Dispose().

Nearest fix

StreamReader reader = new StreamReader(jsonFile);
string json = reader.ReadToEnd();
reader.Dispose()

But here's an issue. If reader.ReadToEnd() cause an exception, the file again will be never closed.

Good fix

StreamReader reader = new StreamReader(jsonFile);
try
{
    string json = reader.ReadToEnd();
}
finally
{
    reader.Dispose()
}

But C# has already implemented this pattern, you can use it with less code written.

using (StreamReader reader = new StreamReader(jsonFile))
{
    string json = reader.ReadToEnd();
}

Also .NET has a shortcut for this classic code pattern.

string json = File.ReadAllText(jsonFile);

Read more: Using objects that implement IDisposable, using statement.

As a summary of above notes the Initialize() method will look like:

public static YoozApi Initialize()
{
    string jsonFile = Path.Combine(Environment.CurrentDirectory, "YoozAPI.json");
    string json = File.ReadAllText(jsonFile);
    return new YoozApi(JsonConvert.DeserializeObject<YoozApiSettings>(json));
}

As you can see, I pass YoozApiSettings to YoozApi constructor.

Here's why.

Initializing HttpClient

DefaultRequestHeaders must be implemented once, right after HttpClient instance creation and before the first request sent.

private readonly HttpClient httpClient;

private YoozApi(YoozApiSettings settings) 
{
    httpClient = new HttpClient();
    httpClient.DefaultRequestHeaders.ExpectContinue = false; // I'm not sure if it needed
    ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12; // Remove this for .NET version 4.6 or higher
    var byteArray = Encoding.ASCII.GetBytes($"{settings.User}:{settings.Secret}");
    httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", Convert.ToBase64String(byteArray));
    httpClient.BaseAddress = new Uri(settings.Url);
}
  • ServicePointManager.SecurityProtocol also can be set once.
  • No reason to render credentials per request as User and Secret never changes.
  • HttpClient.BaseAddress is intended to set a root address to make easier futher requests. With base address new Uri("https://myapi.com") to call https://myapi.com/method you may write like httpClient.GetAsync("method") instead of whole URL.
  • private constructor means you can create the class only through Initialize() method.

Asynchronous programming

All IO apis has thier awaitable methods. It makes easy to write asynchronous code that allows not to block threads while waiting. .Result blocks the calling thread, await not. Read more: Asynchronous programming (especially read about difference between IO-bound work and CPU-bound work there).

Awaitable method is method that returns Task or Task<T>, or any other type that implements awaitable pattern e.g. ValueTask of ValueTask<T> (available in .NET Core 3.1 and newer .NET). Awaitable method may be marked as async, may be not. To detect is the method awaitable look at its returning type, not at async.

To make the await able to work, the method must be implement State Machine pattern. C# compiler can implement the State Machine from a regular method automatically. Just add async keyword. Then await can't be used without async, async has no sense without await inside like State Machine for nothing.

private async Task<ServiceResult<string>> InternalSendAsync(string tabDelimitedPOs)
{
    var result = new ServiceResult<string>();
    using (HttpContent httpContent = new StringContent(tabDelimitedPOs, Encoding.UTF8, "text/plain"))
    using (HttpResponseMessage response = await httpClient.PostAsync("/", httpContent))
    {
        result.StatusCode = response.StatusCode;
        result.FaultMessage = response.ReasonPhrase; // you always have a ReasonPhrase as StatusCode
        result.IsFaulted = !response.IsSuccessStatusCode;

        if (response.IsSuccessStatusCode)
            result.ReturnedObject = await response.Content.ReadAsStringAsync();

        return result;
    };
}

Naming guideline: add Async suffix to the name of awaitable method (recommended by Microsoft, I find it useful too).

In addition to above notes, await unwrap the returning result from Task. So it works like .Result but asynchronously.

You can see that I hid the method with private modifier as if it used only internally then no reason to expose it.

Usage

The reasonable question can apper "How can I use async method from sync method". The general answer: "You can call but you can't await it". For top-level methods that launches asynchronous work async keyword can be used with void returning type`.

Let's assume that you're implementing a Windows Forms app.

private readonly YoozApi api = YoozApi.Initialize();

private async void button1_Click(object sender, EventArgs e)
{
    try
    {
        if (await api.PostOrdersAsync(orders))
        {
            // Success
        }
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
}

Always handle all possible exceptions in async void method otherwise you'll not see the fact of the occured exception. Visually nothing will happen.

The whole example

public class YoozApi
{
    private readonly HttpClient httpClient;

    private YoozApi(YoozApiSettings settings) 
    {
        httpClient = new HttpClient();
        httpClient.DefaultRequestHeaders.ExpectContinue = false;
        ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12;
        var byteArray = Encoding.ASCII.GetBytes($"{settings.User}:{settings.Secret}");
        httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", Convert.ToBase64String(byteArray));
        httpClient.BaseAddress = new Uri(settings.Url);
    }

    public static YoozApi Initialize()
    {
        string jsonFile = Path.Combine(Environment.CurrentDirectory, "YoozAPI.json");
        string json = File.ReadAllText(jsonFile);
        return new YoozApi(JsonConvert.DeserializeObject<YoozApiSettings>(json));
    }

    public async Task<bool> PostOrdersAsync(PurchaseOrders orders)
    {
        string tabDelimitedPOs = orders.GetTabDelimitedString();
        ServiceResult<string> result = await InternalSendAsync(tabDelimitedPOs);

        if (result.IsFaulted)
            Log.RecordToDatabase("ExportReleasedPOs", string.Empty, string.Empty, string.Empty, string.Empty, result.FaultMessage);

        return !result.IsFaulted;
    }

    private async Task<ServiceResult<string>> InternalSendAsync(string tabDelimitedPOs)
    {
        var result = new ServiceResult<string>();
        using (HttpContent httpContent = new StringContent(tabDelimitedPOs, Encoding.UTF8, "text/plain"))
        using (HttpResponseMessage response = await httpClient.PostAsync("/", httpContent))
        {
            result.StatusCode = response.StatusCode;
            result.FaultMessage = response.ReasonPhrase; // you always have a ReasonPhrase as StatusCode
            result.IsFaulted = !response.IsSuccessStatusCode;

            if (response.IsSuccessStatusCode)
                result.ReturnedObject = await response.Content.ReadAsStringAsync();

            return result;
        };
    }
}

Note: Windows Server 2008 R2 doesn't support TLS 1.2 by default but it can be enabled manually through Windows Registry. Anyway I recommend to update the .NET Framework to 4.7.2 as it fully compartible with Server 2008 R2 SP1. The update won't affect other 4.5.2 apps.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you very kindly for this thorough response. Can I follow up and ask how to write a unit test for PostOrdersAsync? I've modified an existing test method of mine and it appears as if it doesn't even run. \$\endgroup\$ Jul 1, 2021 at 20:37
  • \$\begingroup\$ Currently reading this. Please let me know if you have a good reference to read as well. docs.microsoft.com/en-us/archive/msdn-magazine/2014/november/… \$\endgroup\$ Jul 1, 2021 at 21:10
  • \$\begingroup\$ @CodenameCain a unit test unfortunately I didn't write tests a lot. Btw, nice article containing a lot of useful info. \$\endgroup\$
    – aepot
    Jul 1, 2021 at 21:32
  • 1
    \$\begingroup\$ Instead of public void MyTestMethod (not the real name, don't skewer me) I had to change the signature to public async Task MyTestMethod. \$\endgroup\$ Jul 1, 2021 at 21:46

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.