0

I am trying to delete a zip file in DotNetZip as it is like transaction(either do it full or not at all).

Currently, I have incremented my count twice, on purpose to break the app and see if it saves the zip.

Well it does, it saves the zip with 2 files instead of 3 and as I said I want it to behave like a transaction.

  using (ZipFile zip = new ZipFile(outputdirectory))
        {
            var count = 0;

        //    string invoiceNumber = documents[count].GetElementsByTagName("InvoiceNumber")[count].InnerText + ".xml";

            if (documents.Count == 1)
            {
                string invoiceNumber = documents[0].GetElementsByTagName("InvoiceNumber")[0].InnerText + ".xml";
                XmlWriterSettings settings = new XmlWriterSettings();
                settings.Encoding = Encoding.GetEncoding("UTF-8");
                SaveFiles(documents[0], invoiceNumber, settings);
                resultValue = InvoiceResult.Success;
            }
            else
            { 
                foreach (var document in documents)
                {
                    try
                    {
                        string invoiceNumber = documents[count].GetElementsByTagName("InvoiceNumber")[0].InnerText + ".xml";

                    count++;

                        using (MemoryStream memoryStream = new MemoryStream())
                        {
                            document.Save(memoryStream);

                            memoryStream.Seek(0, SeekOrigin.Begin);

                            zip.AddEntry($"{invoiceNumber}.xml", memoryStream);

                                zip.Save();

                        }
                    }
                    catch (Exception)
                    {
                        var wrongZip = ZipFile.Read(outputdirectory);                        
                        wrongZip.Dispose();
                        resultValue = InvoiceResult.CannotCreateZipFile;
                   //     throw new IOException($"Zip file in path {outputdirectory} has already been created. Please delete file if you want to make a new one.");
                    }
                    count++;

                }
            }
        }

I have found this article on Stackoverflow.

DotNetZip how to delete a file after extracting

It said read the stream of the zip and then Dispose of it. I have tried that but the zip file still exists.

No errors, just does not delete the file.

grozdeto
  • 1,201
  • 1
  • 13
  • 34
  • is it okay to just use System.IO.File.Delete(outputdirectory); I mean do I have to read and dispose of the file as well? – grozdeto Nov 13 '19 at 09:40
  • Just use the `System.IO.File.Delete` library to delete the zip AFTER disposing of it in the `DotNetZip`library (not tested) – lordvlad30 Nov 13 '19 at 09:45
  • That's not what that says... it just says the zipfile object needs to be disposed. Just put your try-catch block _around_ the `using` block. You said it needs to fully abort on fail anyway, and then the object is already disposed and you can always safely delete it. – Nyerguds Nov 13 '19 at 09:46
  • By the way... is there any point to saving the zip file in between adding files? Isn't it much simpler to just save once at the end? Then you shouldn't have incomplete zipfiles at all. Or is that because you're using `MemoryStream`s as source for the files? Because that smells a lot like a [Schlemiel The Painter's Algorithm](https://en.wikichip.org/wiki/schlemiel_the_painter%27s_algorithm), since it'll rewrite the whole zipfile each time. – Nyerguds Nov 13 '19 at 09:49

1 Answers1

0

There is quite a lot of chaos in that code; the combination of a counter and a foreach, looped re-saving of the zipfile, unnecessary re-reading of an already-open zipfile...

Since you literally said that any error in the saving process should lead to a full abort, it makes a lot more sense to put the try-catch around the ZipFile's using block. One of the functions of a using block is to clean up the object even in case of an exception, after all.

As for the looped re-saving, personally, I'd simply make an array of MemoryStream objects, and in each loop iteration, fill one in, link it into the ZipFile as file, and save the ZipFile only once, after the whole loop. You can then safely dispose the MemoryStream objects from the array. This way, they all exist simultaneously at the moment you save the ZipFile. And if you only dispose them after the ZipFile's using block you are 100% sure the ZipFile object no longer needs them.

If you do it this way, nothing will ever be written in case an error occurs, since it never gets to the zip.Save() line. So there will be no need to delete anything, either.

try
{
    Int32 docCount = documents.Count;
    MemoryStream[] streams = null;
    using (ZipFile zip = new ZipFile(outputdirectory))
    {
        if (docCount == 1)
        {
            string invoiceNumber = documents[0].GetElementsByTagName("InvoiceNumber")[0].InnerText + ".xml";
            XmlWriterSettings settings = new XmlWriterSettings();
            settings.Encoding = Encoding.GetEncoding("UTF-8");
            SaveFiles(documents[0], invoiceNumber, settings);
            resultValue = InvoiceResult.Success;
        }
        else
        {
            streams = new MemoryStream[docCount]
            // Since we need an index for both the documents and streams array,
            // use 'for' and not 'foreach'
            for (Int32 i = 0; i < docCount; ++i)
            {
                var doc = documents[i];
                string invoiceNumber = doc.GetElementsByTagName("InvoiceNumber")[0].InnerText + ".xml";
                MemoryStream str = new MemoryStream();
                // Store stream in array so you can dispose it later
                streams[i] = str;
                doc.Save(str);
                str.Seek(0, SeekOrigin.Begin);
                zip.AddEntry($"{invoiceNumber}.xml", str);
            }
            zip.Save();
        }
    }
    if (streams != null)
        for (Int32 i = 0; i < docCount; ++i)
            if (streams[i] != null)
                streams[i].Dispose();
}
catch (Exception)
{
    resultValue = InvoiceResult.CannotCreateZipFile;
}

There do seem to be some details missing from this, though. The 1-file logic doesn't seem to use the ZipFile at all, meaning the ZipFile's using block can technically just be placed inside the else. Also, the multiple-file logic never sets the resultValue.

Nyerguds
  • 5,360
  • 1
  • 31
  • 63