Sending GET/POST requests in .NET

Question

I have a RequestsExtensions class with two functions for sending GET/POST requests.

using System;
using System.Net;
using System.Text;
 public static class RequestsExtensions
 {
 /// <summary>
 /// Sending POST request.
 /// </summary>
 /// <param name="Url">Request Url.</param>
 /// <param name="Data">Data for request.</param>
 /// <returns>Response body.</returns>
 public static string HTTP_POST(string Url, string Data)
 {
 string Out = String.Empty;
 System.Net.WebRequest req = System.Net.WebRequest.Create(Url);
 try
 {
 req.Method = "POST";
 req.Timeout = 100000;
 req.ContentType = "application/x-www-form-urlencoded";
 byte[] sentData = Encoding.UTF8.GetBytes(Data);
 req.ContentLength = sentData.Length;
 using (System.IO.Stream sendStream = req.GetRequestStream())
 {
 sendStream.Write(sentData, 0, sentData.Length);
 sendStream.Close();
 }
 System.Net.WebResponse res = req.GetResponse();
 System.IO.Stream ReceiveStream = res.GetResponseStream();
 using (System.IO.StreamReader sr = new System.IO.StreamReader(ReceiveStream, Encoding.UTF8))
 {
 Char[] read = new Char[256];
 int count = sr.Read(read, 0, 256);
 while (count > 0)
 {
 String str = new String(read, 0, count);
 Out += str;
 count = sr.Read(read, 0, 256);
 }
 }
 }
 catch (ArgumentException ex)
 {
 Out = string.Format("HTTP_ERROR :: The second HttpWebRequest object has raised an Argument Exception as 'Connection' Property is set to 'Close' :: {0}", ex.Message);
 }
 catch (WebException ex)
 {
 Out = string.Format("HTTP_ERROR :: WebException raised! :: {0}", ex.Message);
 }
 catch (Exception ex)
 {
 Out = string.Format("HTTP_ERROR :: Exception raised! :: {0}", ex.Message);
 }
 return Out;
 }
 /// <summary>
 /// Sending GET request.
 /// </summary>
 /// <param name="Url">Request Url.</param>
 /// <param name="Data">Data for request.</param>
 /// <returns>Response body.</returns>
 public static string HTTP_GET(string Url, string Data)
 {
 string Out = String.Empty;
 System.Net.WebRequest req = System.Net.WebRequest.Create(Url + (string.IsNullOrEmpty(Data) ? "" : "?" + Data));
 try
 {
 System.Net.WebResponse resp = req.GetResponse();
 using (System.IO.Stream stream = resp.GetResponseStream())
 {
 using (System.IO.StreamReader sr = new System.IO.StreamReader(stream))
 {
 Out = sr.ReadToEnd();
 sr.Close();
 }
 }
 }
 catch (ArgumentException ex)
 {
 Out = string.Format("HTTP_ERROR :: The second HttpWebRequest object has raised an Argument Exception as 'Connection' Property is set to 'Close' :: {0}", ex.Message);
 }
 catch (WebException ex)
 {
 Out = string.Format("HTTP_ERROR :: WebException raised! :: {0}", ex.Message);
 }
 catch (Exception ex)
 {
 Out = string.Format("HTTP_ERROR :: Exception raised! :: {0}", ex.Message);
 }
 return Out;
 }
 }

Do you see any improvement / issue?

Answer 1

First off your methods are not extension methods as the name RequestsExtensions would indicate. Not sure if this intentional or. If you want to make them true extension methods then I'd consider passing the Url parameter as Uri rather than as string.

Other things I noticed:

1. The use of a magic number here: req.Timeout = 100000; - This should be a parameter (maybe with a default value) you can pass in.

2. Your naming convention is a bit weird. Standard naming convention for methods in C# is PascalCase. Also local variables are normally camelCase (your Out variable is not while other variables like req are).

3. In HTTP_POST you use a block-wise read to get the response while in HTTP_GET you simply read to the end of the response stream - why the difference? The HTTP_GET version seems much simpler so why not do it the same way? In fact both those method get a response and read it and deal with the exceptions - this is duplicated code which you should move into a dedicated function which you can then call from both main methods like this:

   private static string GetResponse(System.Net.WebRequest request)
   {
    string result = "";
    try
    {
    System.Net.WebResponse resp = req.GetResponse();
    using (System.IO.Stream stream = resp.GetResponseStream())
    {
    using (System.IO.StreamReader sr = new System.IO.StreamReader(stream))
    {
    result = sr.ReadToEnd();
    sr.Close();
    }
    }
    }
    catch 
    ....
    return result;
   }
   

4. The way you handle exceptions is less than ideal. The caller of your methods has no easy way to find out if the call actually succeeded or not - except for parsing the return value of the method.

   A better way to deal with it is to let exceptions bubble up to the caller and let the caller decide what to do (e.g. terminate the process, retry later, etc.). In addition you could log the exception (requires passing in some kind of logger)

   Alternatively you could make the return value an out parameter and return a bool indicating if the call was successful instead if you really want to hide the exceptions from the caller. In this case I'd rename the methods to something like TryHttpPost and TryHttpGet

• Jesse C. Slicer: As a note, sr.Close(); is unnecessary since the creation of the StreamReader is in a using block.

Answer 2

When something goes wrong, your method should throw an exception, that's what they're for. It shouldn't just return an error message instead of the normal response. That way, the calling code can easily differentiate between success and error.

• sDima: For example, when user sending email from contact form I use HttpGet request for getting information from geoip service and adding this to email text. And if geoip service will be unavailable this method will throw exception and user can't send email.
• svick: @sDima This method will throw exception and the method that calls this method can catch it.

Answer 3

If the only purpose is to provide a simple wrapper around the body of the HTTP response, this isn't so bad. Things break down when you need to access things like the request and response headers.

Error handling is another thing entirely. I would recommend catching all errors, and rethrowing the exceptions wrapped in your own exception classes to make error handling for client code easier:

public class RequestError : Exception
{
 ...
}
public class RequestNotFoundError : RequestError
{
 ...
}
public class RequestServerEror : RequestError
{
}

The client code becomes much clearer:

try
{
 RequestExtensions.HTTP_POST(...)
}
catch (RequestNotFoundError)
{
 // 404 Not Found
}
catch (RequestServerError)
{
 // 500 Internal Server Error
}
catch (RequestError)
{
 // Some sort of unhandled HTTP error
}
catch (Exception)
{
 // Gremlins, I suppose
}

The hardest part of managing HTTP requests is that there are a million ways for these things to die. Any assistance you can give for error handling will be appreciated by all who use this class.

Answer 4

I'm not sure I'd totally approve of:

The second HttpWebRequest object has raised an Argument Exception as 'Connection' Property is set to 'Close'

How have you determined this? in my opinion the message could be incorrect and be an exception for a totally different reason.

Your other two catches are far more generic, which is likely better, also the latter two catch both end in an exclamation mark (which I don't like!(meta!)) but if you're going to do it, do keep it consistent across all three exceptions!