2
\$\begingroup\$

This method is supposed to make a custom requests and then get its response. Sometimes, the response is a string, and most the time the response is in JSON format. As far as I know I can't have a function which returns either a string or a json format. So because of that I have the same method twice but one returns a string and other the json format. (This could be solved with a void return's type maybe? not sure tho).

This is the method code. I'm interested not only in fixing the problem of the types that the function returns but also in the function itself, perhaps it could be improved. Any advice is welcome. Thanks!

async Task<string> Request_2(HttpMethod method, string endpoint, string query = "", string data = "")  {     
   HttpRequestMessage request;
   HttpResponseMessage response;

   // If it has query
   if (query.Length > 0)
      request = new HttpRequestMessage(method, URL + endpoint + "?" + query);
   else
      request = new HttpRequestMessage(method, URL + endpoint);

   // Encode USER:PASS for the HTTP Basic
   string svcCredentials = Convert.ToBase64String(ASCIIEncoding.ASCII.GetBytes("riot:" + PASS));
             
   // Skip SSL
   ServicePointManager.ServerCertificateValidationCallback += (sender, cert, chain, sslPolicyErrors) => true;

   // Headers
   request.Headers.Add("Authorization", "Basic " + svcCredentials);

   if (data.Length > 0)
      request.Content = new StringContent(data, Encoding.UTF8, "application/json");

   response = await client.SendAsync(request);
   return await response.Content.ReadAsStringAsync();           
}

```
\$\endgroup\$
4
  • 1
    \$\begingroup\$ 1) Skip SSL unacceptable as a huge security breach 2) HttpWebRequest is deprecated many years ago, use HttpClient instead 3) use asynchronous programming async/await while dealing with I/O operations. 4) totally ignored IDisposables, read this. The Code Review for proposed solution is not applicable while it's built on outdated .NET APIs. \$\endgroup\$
    – aepot
    Commented Jun 11, 2021 at 18:27
  • \$\begingroup\$ HttpClient docs \$\endgroup\$
    – aepot
    Commented Jun 11, 2021 at 18:33
  • \$\begingroup\$ I've updated the post with the HttpClient library. Skip SSL is a must right now till I get the certificate. (The request are always pointing to the same site, and is secure tho...) What about now? Thanks! @aepot \$\endgroup\$
    – Sharki
    Commented Jun 11, 2021 at 19:31
  • 1
    \$\begingroup\$ Skip SSL is a must right now till I get the certificate. use HTTP then as a fallback. ServicePointManager doen't make any effect on HttpClient in .NET Core/.NET 5. Also you may fix mistakes in the code before posting it here (read it twice and you'll find ones). Still ignored IDisposable. async void - totally bad practice. Please read the docs I posted in the above comments. Anyway, this one looks better than initial solution. \$\endgroup\$
    – aepot
    Commented Jun 11, 2021 at 19:47

1 Answer 1

1
\$\begingroup\$
  • Don't bypass Certificate validation. You may install development certificates provided by Microsoft for debugging.
  • Use suffix Async for Task-returning (awaitable) methods. It makes code more readable.
  • As far as I can see, the solution is intended to simplify network-related code which requires HTTP API interactions. Then encapsulate the solution into separate class to keep the state inside.
  • To make it more easy to use, split GET and POST implementations in two separate methods

Let's assume that you're using .NET Core 3.1 or newer .NET e.g. 5:

public class ApiHelper : IDisposable
{
    private readonly HttpClient _client;

    public ApiHelper(string baseUrl, string login, string password)
    {
        _client = new HttpClient
        {
            DefaultRequestVersion = new Version(2, 0),  // support HTTP/2
            BaseAddress = new Uri(baseUrl) // providing root for all requests
        };
        string credentials = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{login}:{password}"));
        _client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials); // all requests will be sent with this header
    }

    public Task<string> GetAsync(string method, Dictionary<string, string> args = null)
    {
       if (args?.Count > 0)
       {
           string query = new FormUrlEncodedContent(args).ReadAsStringAsync().Result; // no need await here because the content is already inside, then ReadAsStringAsync() completes synchronously
           method += $"?{query}";
       }
       return _client.GetStringAsync(method);
    }

    public async Task<string> PostStringAsync(string method, string data)
    {
        using HttpContent content = new StringContent(data, Encoding.UTF8, "application/json"); 
        using var response = await _client.PostAsync(method, content).ConfigureAwait(false);
        return await response.EnsureSuccessStatusCode().Content.ReadAsStringAsync(); 
    }

    public Dispose() => _client.Dispose();
}

Usage might be clear.

private ApiHelper _api = new ApiHelper("https://myserver/api", "riot", pass);

async Task GetExampleAsync()
{
    var args = new Dictionary<string, string>
    {
        ["param1"] = "value1",
        ["param2"] = "value2"
    };
    // https://myserver/api/apimethod?param1=value1&param2=value2
    string response = await _api.GetAsync("apimethod", args);
}

async Task PostExampleAsync()
{
    // { "msg": "Hello World" }
    string json = JsonSerializer.Serialize(new { msg = "Hello World" });
    string response = await _api.PostStringAsync("apimethod", json);
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for your reply @aepot, I really appreciate it. I feel kinda bad knowing that my whole code sucks to the point that you had to rewrite it entirely. This is kinda off-topic but, if it isn't too much to ask, what tips/advice would you give me to become as good as you are? \$\endgroup\$
    – Sharki
    Commented Jun 12, 2021 at 18:12
  • 1
    \$\begingroup\$ @Sharki carefully read the docs as per links I provided in the comments (I know that Microsoft is writing hard to read docs but it worth to ignore that and anyway read it). Then learn, how async/await works. Not just correct use and bad/best practices but internally. The same "how it internally works" recipe works for anything. A Github .NET repo may help to learn. \$\endgroup\$
    – aepot
    Commented Jun 12, 2021 at 18:40

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.