Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Does my code properly clean up its List<MemoryStream>?

I've got a third-party component that does PDF file manipulation. Whenever I need to perform operations I retrieve the PDF documents from a document store (database, SharePoint, filesystem, etc.). To make things a little consistent I pass the PDF documents around as a byte[].

This 3rd party component expects a MemoryStream[] (MemoryStream array) as a parameter to one of the main methods I need to use.

I am trying to wrap this functionality in my own component so that I can use this functionality for a number of areas within my application. I have come up with essentially the following:

public class PdfDocumentManipulator : IDisposable
{
   List<MemoryStream> pdfDocumentStreams = new List<MemoryStream>();

   public void AddFileToManipulate(byte[] pdfDocument)
   {
      using (MemoryStream stream = new MemoryStream(pdfDocument))
      {
         pdfDocumentStreams.Add(stream);
      }
   }

   public byte[] ManipulatePdfDocuments()
   {
      byte[] outputBytes = null;

      using (MemoryStream outputStream = new MemoryStream())
      {
           ThirdPartyComponent component = new ThirdPartyComponent();
           component.Manipuate(this.pdfDocumentStreams.ToArray(), outputStream);

           //move to begining
           outputStream.Seek(0, SeekOrigin.Begin);

           //convert the memory stream to a byte array
           outputBytes = outputStream.ToArray();
      }

      return outputBytes;
   }

   #region IDisposable Members
   public void Dispose()
   {
       for (int i = this.pdfDocumentStreams.Count - 1; i >= 0; i--)
       {
          MemoryStream stream = this.pdfDocumentStreams[i];
          this.pdfDocumentStreams.RemoveAt(i);
          stream.Dispose();
       }
   }
   #endregion
}

The calling code to my "wrapper" looks like this:

    byte[] manipulatedResult = null;
    using (PdfDocumentManipulator manipulator = new PdfDocumentManipulator())
    {
        manipulator.AddFileToManipulate(file1bytes);
        manipulator.AddFileToManipulate(file2bytes);
        manipulatedResult = manipulator.Manipulate();
    }

A few questions about the above:

  1. Is the using clause in the AddFileToManipulate() method redundant and unnecessary?
  2. Am I cleaning up things OK in my object's Dispose() method?
  3. Is this an "acceptable" usage of MemoryStream? I am not anticipating very many files in memory at once...Likely 1-10 total PDF pages, each page about 200KB. App designed to run on server supporting an ASP.NET site.
  4. Any comments/suggestions?

Thanks for the code review :)

like image 202
Brian Avatar asked Dec 17 '22 08:12

Brian


1 Answers

AddFileToManipulate scares me.

   public void AddFileToManipulate(byte[] pdfDocument)
   {
      using (MemoryStream stream = new MemoryStream(pdfDocument))
      {
         pdfDocumentStreams.Add(stream);
      }
   }

This code is adding a disposed stream to your pdfDocumentStream list. Instead you should simply add the stream using:

   pdfDocumentStreams.Add(new MemoryStream(pdfDocument));

And dispose of it in the Dispose method.

Also you should look at implementing a finalizer to ensure stuff gets disposed in case someone forgets to dispose the top level object.

like image 196
Sam Saffron Avatar answered Jan 10 '23 19:01

Sam Saffron