Generic WebRequest Method for .PCL

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?

/// 
    /// Test-Method
    ///        
    /// Json Content String
    /// 
    public static T CreateRequest(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(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

Thing that makes me more perplex is error-handling.

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

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

• 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:

• Prepares the request.

• Gets the response.

• 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(string url, HttpMethod method, string payload) {
    var request = CreateRequest(url, method, payload);
    var response = ReadResponseAsString(request);

    return JsonConvert.DeserializeObject(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.

StackExchange Code Review Q#140278, answer score: 5