2
\$\begingroup\$

At the moments the requirements do not need me to change this code, however I think there is something wrong with it in the fact that it seems to know too much. The code takes a memory stream which is essentially a file and uploads this to a post URL supplying the necessary credentials. It may also need to pass other form value fields which is available in the NameValuecollection parameter.

For me it seems this class breaks a number of different SOLID principles so I'm looking for ways to make it better from this point of view.

Some things I'm not so sure about and how to possibly make better are:

  1. Having it not accept credentials and be responsible for adding this to the HttpWebRequest. What happened if I wanted to change the way the credentials were used. The class would need to change for this.

  2. The class is responsible for building up the posted stream and also sending it. Was thinking that the sending shouldn't be a part of this process.

  3. What happens if I want to send multiple files. At the moment the code does not support that but I'm sure there's a way I could make it generic enough to support multiple file upload.

Some things I am considering is:

  1. Passing in a wrapper class for the upload memory stream that would contain it's Stream, FileName and Content-Type. Maybe something like List.

  2. Having another class be responsible for the sending of the data stream, so renaming the Upload to something like GetUploadStream().

  3. An interface perhaps for doing the credentials, or maybe inject that into another class that is the one doing the actual upload?

I don't want to make this more complex than it needs to be however to me at the moment it just feels too rigid.

Onwards and upwards, here's the code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.IO;
using System.Collections.Specialized;
using System.Net;
using System.Globalization;

namespace CDAX.Schema.Zip
{
    public class HttpZipUpload
    {
        private ICredentials _credentials;
        private readonly string _boundary = "";

        public HttpZipUpload(ICredentials credentials)
        {
            _boundary = "---------------------------" + DateTime.Now.Ticks.ToString("x", NumberFormatInfo.InvariantInfo);
            _credentials = credentials;
        }

        public byte[] Upload(string url, MemoryStream dataStream)
        {
            return Upload(url, dataStream, new NameValueCollection());
        }

        public byte[] Upload(string url, MemoryStream dataStream, NameValueCollection nameValues)
        {
            HttpWebRequest request = GetHttpWebRequest(url);
            WriteAuthenticationToRequest(request);

            dataStream.Position = 0;
            using (MemoryStream outputStream = new MemoryStream())
            {
                WriteNameValuesToStream(outputStream, nameValues);

                WriteDataContentToStream(outputStream, dataStream);

                WriteToHttpStream(request, outputStream);
            }

            // return the response
            return GetHttpWebResponse(request);
        }

        private byte[] GetHttpWebResponse(HttpWebRequest request)
        {
            using (var response = request.GetResponse())
            {
                using (var responseStream = response.GetResponseStream())
                {
                    using (var stream = new MemoryStream())
                    {
                        responseStream.CopyTo(stream);
                        return stream.ToArray();
                    }
                }
            }
        }

        private HttpWebRequest GetHttpWebRequest(string url)
        {
            HttpWebRequest request = (HttpWebRequest)WebRequest.Create(url);
            request.ContentType = string.Format("multipart/form-data; boundary={0}", _boundary);
            request.Method = "POST";

            return request;
        }

        private void WriteAuthenticationToRequest(HttpWebRequest request)
        {
            var user = _credentials.GetCredential(request.RequestUri,"Basic");
            string auth = string.Format("{0}:{1}", user.UserName, user.Password);

            request.Headers.Add("Authorization", "Basic " + Convert.ToBase64String(Encoding.ASCII.GetBytes(auth)));
        }

        private void WriteEndBoundaryToStream(MemoryStream stream)
        {
            WriteBoundaryToStream(stream, "--");
        }

        private void WriteBoundaryToStream(MemoryStream stream, string endDeliminator)
        {
            WriteToStream(stream, Encoding.ASCII, string.Format("--{0}{1}", _boundary, endDeliminator));
        }

        private void WriteDataContentToStream(MemoryStream outputStream, MemoryStream inputStream)
        {
            // write content boundary start
            WriteBoundaryToStream(outputStream, Environment.NewLine);

            string formName = "uploaded";
            string fileName = "input.zip";
            string contentType = "application/zip";

            WriteToStream(outputStream, System.Text.Encoding.UTF8, string.Format("Content-Disposition: form-data; name=\"{0}\"; filename=\"{1}\"{2}", formName, fileName, Environment.NewLine));
            WriteToStream(outputStream, System.Text.Encoding.UTF8, string.Format("Content-Type: {0}{1}{1}", contentType, Environment.NewLine));

            byte[] buffer = new byte[inputStream.Length];
            int bytesRead = 0;

            while ((bytesRead = inputStream.Read(buffer, 0, buffer.Length)) != 0)
            {
                outputStream.Write(buffer, 0, bytesRead);
            }

            // must include a new line before writing the end boundary
            WriteToStream(outputStream, Encoding.ASCII, Environment.NewLine);

            // make sure we end boundary now as the content is finished
            WriteEndBoundaryToStream(outputStream);
        }

        private void WriteNameValuesToStream(MemoryStream stream, NameValueCollection nameValues)
        {
            foreach (string name in nameValues.Keys)
            {
                WriteBoundaryToStream(stream, Environment.NewLine);

                WriteToStream(stream, Encoding.UTF8, string.Format("Content-Disposition: form-data; name=\"{0}\"{1}{1}", name, Environment.NewLine));
                WriteToStream(stream, Encoding.UTF8, nameValues[name] + Environment.NewLine);
            }
        }

        private void WriteToHttpStream(HttpWebRequest request, MemoryStream outputStream)
        {
            outputStream.Position = 0;

            byte[] tempBuffer = new byte[outputStream.Length];
            outputStream.Read(tempBuffer, 0, tempBuffer.Length);

            request.ContentLength = outputStream.Length;

            using (Stream requestStream = request.GetRequestStream())
            {
                requestStream.Write(tempBuffer, 0, tempBuffer.Length);
                requestStream.Close();
            }
        }

        private void WriteToStream(MemoryStream stream, Encoding encoding, string output)
        {
            byte[] headerbytes = encoding.GetBytes(output);
            stream.Write(headerbytes, 0, headerbytes.Length);
        }

    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Quick small notes: See the answer to: stackoverflow.com/questions/2845055/… Also in the WriteToHttpStream, your using body is too large. It only needs to wrap around a single line: requestStream.Write. You do not really need to explicitly requestStream.Close. However, outputStream could misbehave as well, but I suppose it is up to the caller to clean up that mess. Thirdly, run StyleCop on this, and finally - your class is relatively small. I would only start to change it if you really ARE GOING TO NEED IT. I trust you can refactor it later. \$\endgroup\$ – Leonid Mar 18 '12 at 17:08
  • \$\begingroup\$ @Leonid Thanks for tips.I've changed the WriteToHttpStream() to your suggestion. I know I could refactor later but just looking for some code review on it now while it's fresh in my mind \$\endgroup\$ – dreza Mar 18 '12 at 20:13
  • \$\begingroup\$ So I ran StyleCop over this code but didn't really come up with too much based on the configuration options I have. Good little tool though. \$\endgroup\$ – dreza Mar 19 '12 at 20:29
2
\$\begingroup\$

The code looks ok, but indeed there are too many responsibilities in a single class, hence SRP is violated.

For instance, you could use a decorator to add the credentials, instead of having them injected in the constructor. But this leads to your own model of HTTP request: the headers, the body, everything. Once you have the model, you can play around with it as much as you want, set everything (credentials, boundary, etc.) and eventually invoke the relevant method, which, in turn, translates your model to the .NET's HTTP request.

So... constructing a HTTP request the SOLID way, using OOP and design patterns is not that trivial. I'd suggest you take a look at RestSharp, which aims for REST, but is a very versatile HTTP client in itself.

| improve this answer | |
\$\endgroup\$

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.