4
\$\begingroup\$

I have created this method to do all my WebRequests.

Generally my question is, if this could be improved in any way. As a side question, I would like to know if it be better to create methods for each type (PUT, POST, GET, UPDATE, DELETE) of a request separately?

    /// <summary>
    /// Test-Method
    /// </summary>       
    /// <param name="method">Json Content String</param>
    /// <param name="postData"></param>
    public static T CreateRequest<T>(string url, ICredentials cred, string method, string postData = null) where T : class
    {
        string responseJson = string.Empty;
        try
        {
            //Do request:
            HttpWebRequest request = WebRequest.Create(url) as HttpWebRequest;
            request.Method = method.ToUpper();//Just in case ToUpper();
            request.ContentType = "application/json";

            if ((method == "PUT" || method == "POST") && postData != null)
            {
                using (var streamWriter = new StreamWriter(request.GetRequestStream()))
                {
                    streamWriter.Write(postData);
                    streamWriter.Flush();
                }
            }                

            var httpResponse = (HttpWebResponse)request.GetResponse();
            using (StreamReader reader = new StreamReader(httpResponse.GetResponseStream()))
            {
                responseJson = reader.ReadToEnd();
            }
        }
        catch (Exception ex)
        {
            Debug.Fail($"WebRequest failed: {ex.Message}");
            return null;
        }

        try
        {
            return JsonConvert.DeserializeObject<T>(responseJson);
        }
        catch (Exception ex)
        {
            Debug.Fail($"Error parsing JSON: {ex.Message}");
            return null;
        }
    }

Note:

  • credentials excluded for this purpose
  • don't focus on exceptionhandling- I left that out too
| improve this question | | | | |
\$\endgroup\$
1
\$\begingroup\$

Helper class Xamarin PCL compatible version:

public static class JsonRestApi
{
    public static Task<T> GetAsync<T>(this Uri url) where T : class =>
        url
            .CreateRequest(HttpMethod.Get)
            .ParseJsonAsync<T>();

    public static async Task<T> PostAsync<T>(this Uri url, object data = null) =>
        await (await url
            .CreateRequest(HttpMethod.Post)
            .SendDataAsync(data))
            .ParseJsonAsync<T>();

    // etc...

    static HttpWebRequest CreateRequest(this Uri url, HttpMethod method)
    {
        var request = WebRequest.Create(url) as HttpWebRequest;
        request.Method = method.ToString().ToUpperInvariant();
        request.ContentType = "application/json";
        return request;
    }

    static async Task<HttpWebRequest> SendDataAsync(this HttpWebRequest request, object data)
    {
        if (data != null)
            using (var writer = new StreamWriter(await request.GetRequestStreamAsync()))
                writer.Write(JsonConvert.SerializeObject(data));

        return request;
    }

    static async Task<T> ParseJsonAsync<T>(this HttpWebRequest request)
    {
        using (var response = (HttpWebResponse)await request.GetResponseAsync())
        using (StreamReader reader = new StreamReader(response.GetResponseStream(), response.GetEncoding()))
            return JsonConvert.DeserializeObject<T>(reader.ReadToEnd());
    }

    static Encoding GetEncoding(this HttpWebResponse response) =>
        Encoding.GetEncoding(response.CharacterSet());

    static string CharacterSet(this HttpWebResponse response)
    {
        if (string.IsNullOrWhiteSpace(response.ContentType))
            return string.Empty;

        string srchString = response.ContentType.ToLowerInvariant();

        //media subtypes of text type has a default as specified by rfc 2616
        if (srchString.Trim().StartsWith("text/"))
            return "ISO-8859-1";

        //one of the parameters may be the character set
        //there must be at least a mediatype for this to be valid
        var characterSet = string.Empty;
        int i = srchString.IndexOf(";");
        if (i > 0)
        {
            //search the parameters
            while ((i = srchString.IndexOf("charset", i)) >= 0)
            {
                i += 7;
                //make sure the word starts with charset
                if (srchString[i - 8] == ';' || srchString[i - 8] == ' ')
                {
                    //skip whitespace
                    while (i < srchString.Length && srchString[i] == ' ')
                        i++;

                    //only process if next character is '='
                    //and there is a character after that
                    if (i < srchString.Length - 1 && srchString[i] == '=')
                    {
                        i++;

                        //get and trim character set substring
                        int j = srchString.IndexOf(';', i);
                        //In the past we used
                        //Substring(i, j). J is the offset not the length
                        //the qfe is to fix the second parameter so that this it is the
                        //length. since j points to the next ; the operation j -i
                        //gives the length of the charset
                        if (j > i)
                            characterSet = response.ContentType.Substring(i, j - i).Trim();
                        else
                            characterSet = response.ContentType.Substring(i).Trim();

                        //done
                        break;
                    }
                }
            }
        }

        return characterSet;
    }
}
| improve this answer | | | | |
\$\endgroup\$
5
\$\begingroup\$

Thing that makes me more perplex is error-handling.

  1. First of all you're catching all exceptions, it may hide subtle bugs unrelated to web request itself (for example wrong parameters you supply in your own code, StackoverflowException, OutOfMemoryException and so on).
  2. You're swallowing exceptions, you just log them and return null (which is little bit counter-sense because it nullify any benefit of exceptions itself).
  3. You do not give any chance to caller to handle exceptions (you're not doing anything useful, they may do), for example retry after a short amount of time (in case network error is transitory) or even to have any meaningful information about error.

IMO if you're writing library reusable code you shouldn't, it may be acceptable if this is an helper method to avoid repetitive code (but in this case some decent error handling is mandatory). Take, at least, a look at System.Net.WebException: The remote name could not be resolved.


After that, starting from top, I have some perplexity about method argument. It's a string and it has two drawbacks:

  • User may pass any unknown method and you won't know. Server may respond with an error 501 (according to RFC 2616 section 5.1.1, superseded by RFC 5789) but you will even get 400, 404 or probably 405 out there in the big real world...
  • User has always to type an error-prone string, no compile-time checks: errors will be detected at run-time (what if she types OPTONS instead of OPTIONS?)

Moreover you're handling strings in the wrong way:

  • You have method.ToUpper() just in case but there should not be a just in case in production code! Throw an error if you want user to use uppercase text or smoothly handle case errors. According to RFC 2616 (again in section 5.1.1) HTTP method is case sensitive (even if some servers don't care).
  • If a Turkish user enters options your code will fail because ToUpper() (again it's culture aware) will convert it to OPTİONS (note dotted uppercase İ instead of I).
  • You're comparing strings with == and it's current culture aware (same as above, it's not safe) and it's not case-insensitive (then what's the point of previous ToUpper()?)

What I'd do? Simply avoid strings in favor of an enum:

public enum HttpMethod
{
    Get,
    Post,
    Put,
}

Then you can convert that to string:

request.Method = HttpMethodToString(method);
if ((method == HttpMethod.Put || method == HttpMethod.Post) && postData != null)

Where HttpMethodToString() may (unless you want to use different names and enum items decorated with attributes) simply be:

static string HttpMethodToString(HttpMethod method)
{
    return method.ToString().ToUpperInvariant();
}

Even better: use already existing System.Web.Http.HttpMethod class and directly use it as string:

request.Method = method.ToString();
if ((method == HttpMethod.Put || method == HttpMethod.Post) && postData != null)

Now let's go to request itself.

HttpWebResponse implements IDisposable then it must be properly disposed, enclose it inside an using statement.

You're assuming that HTTP response is encoded as UTF-8 (StreamReader default) but it's not always true (I don't see anywhere code to add Accept-Charset HTTP header) then your code will fail if server will respond with any other encoding but UTF-8 or - and you're lucky with ISO 8859-1).

Read HttpWebResponse.CharacterSet, create the right Encoding with Encoding.GetEncoding() and give it to StreamReader constructor.


Your function is now doing three things:

  1. Prepares the request.
  2. Gets the response.
  3. Parses response.

It is doing those things inside just one function called CreateRequest, little bit too much (IMO) and with a misleading name! I'd split this function into two/three smaller functions:

public static T ReadJsonFrom<T>(string url, HttpMethod method, string payload) {
    var request = CreateRequest(url, method, payload);
    var response = ReadResponseAsString(request);

    return JsonConvert.DeserializeObject<T>(response);
}

To fill those function is a trivial exercise. Now to understand how it works you need to read just three lines of code and you can put your error-handling code in one specific point: you may, for example, apply a retry pattern to read the response in ReadResponseAsString() but CreateRequest() may fail immediately.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ thanks alot I will apply your suggestions. They make alot of sense ^^ \$\endgroup\$ – Felix D. Sep 2 '16 at 8:55
  • \$\begingroup\$ Thank you, you may also want to wait little bit more before accepting this answer. On Code Review a question has a longer lifespan than, say, Stack Overflow and more answers may be posted! \$\endgroup\$ – Adriano Repetti Sep 2 '16 at 8:58
  • \$\begingroup\$ oh okay... :D I just used to SO ^^ so shall I uncheck this answer ? \$\endgroup\$ – Felix D. Sep 2 '16 at 8:58
  • \$\begingroup\$ and do you think it is generally a good idea to do all the requests with this method or is it better to do them seperately ? \$\endgroup\$ – Felix D. Sep 2 '16 at 9:00
  • 1
    \$\begingroup\$ If possible then yes, I always try to use this helper methods to avoid repetitive code (without abusing them, they're bad for other reasons...testing!) Advantage is that if you need to change something (better error handling? logging?) you have to do it only in one point. Disadvantage is that they're less generic than plain code and adding more and more features may result in a messy function prototype (add more overloads!) \$\endgroup\$ – Adriano Repetti Sep 2 '16 at 9:03
3
\$\begingroup\$

I would define it as an extension to the Uri type, so it could be used this way:

class Program
{
    static void Main(string[] args)
    {
        var blogPosts = Typicode.Posts();
        var blogPost = Typicode.Posts(1);
    }
}

// see https://jsonplaceholder.typicode.com/
// this site exposes some REST API example to automate
public static class Typicode
{
    public static BlogPost[] Posts() =>
        new Uri("https://jsonplaceholder.typicode.com/posts")
            .Get<BlogPost[]>();

    public static BlogPost Posts(int id) =>
        new Uri($"https://jsonplaceholder.typicode.com/posts/{id}")
            .Get<BlogPost>();
}

public class BlogPost
{
    public int UserId { get; set; }
    public int Id { get; set; }
    public string Title { get; set; }
    public string Body { get; set; }
}

Where helper code is:

public static class JsonRestApi
{
    public static T Get<T>(this Uri url) where T: class =>
        url
            .CreateRequest(HttpMethod.Get)
            .ParseJson<T>();

    public static T Post<T>(this Uri url, object data = null) =>
        url
            .CreateRequest(HttpMethod.Post)
            .SendData(data)
            .ParseJson<T>();

    // etc...

    static HttpWebRequest CreateRequest(this Uri url, HttpMethod method)
    {
        var request = WebRequest.Create(url) as HttpWebRequest;
        request.Method = method.ToString().ToUpperInvariant();
        request.ContentType = "application/json";
        return request;
    }

    static HttpWebRequest SendData(this HttpWebRequest request, object data)
    {
        if (data != null)
            using (var writer = new StreamWriter(request.GetRequestStream()))
                writer.Write(JsonConvert.SerializeObject(data));

        return request;
    }

    static T ParseJson<T>(this HttpWebRequest request)
    {
        using (var response = (HttpWebResponse)request.GetResponse())
        using (StreamReader reader = new StreamReader(response.GetResponseStream(), response.GetEncoding()))
            return JsonConvert.DeserializeObject<T>(reader.ReadToEnd());
    }

    static Encoding GetEncoding(this HttpWebResponse response) =>
        Encoding.GetEncoding(response.CharacterSet);
}
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ i like this solution very much. But im unable to implement it in my Xamarin forms PCL ;( \$\endgroup\$ – Felix D. Sep 4 '16 at 13:32
  • 1
    \$\begingroup\$ @Motivated Here it is - please see another answer :) \$\endgroup\$ – Dmitry Nogin Sep 4 '16 at 14:57
  • \$\begingroup\$ thx so much i will have a look on it \$\endgroup\$ – Felix D. Sep 4 '16 at 19:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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