-1
\$\begingroup\$

This code checks if a mail message contains a single page PDF attachment. If yes, it adds it to a processed list else rejected list.

If mail can be processed, then it saves the attachment into a file.

I feel this method is doing more than what it needs to do.

public class Client
{
    #region Private variables
    private ExchangeService _service;        
    #endregion

    public void MoveSinglePagePdfToFolder(List<MailMessage> messagesWithSinglePdf)
    {
    var processed = new List<MailMessage>();
    var rejected = new List<MailMessage>();

    if (messagesWithSinglePdf != null)
    {
        foreach (var message in messagesWithSinglePdf)
        {
                _service.GetAttachments(message.Attachments.ToArray(), BodyType.HTML, null);

                foreach (var attachment in message.Attachments)
                {
                    var fileAttach = (FileAttachment) attachment;
                    if (fileAttach.Name.EndsWith(".pdf"))
                    {
                        var reader = new PdfReader(fileAttach.Content);
                        int numberOfPages = reader.NumberOfPages;

                        //save off
                        if (numberOfPages == 1)
                        {
                            var fileContents = !IsEditablePdf(fileAttach.Content) ? fileAttach.Content : GetUnEditablePdf(fileAttach.Content);

                            // write the file to working directory
                            // move the message to Processed folder
                            var fileName = "fax" + "_" + DateTime.Now.Date.ToShortDateString() + "_" + fileAttach.Name + "_" + DateTime.Now.Ticks + ".pdf";

                            // store the file name in ema directory.
                            File.WriteAllBytes(Statics.EmaFileWatcherWorkingDirectory + fileName, fileContents);
                            processed.Add(message);
                        }

                        else rejected.Add(message);
                    }
                }
            }

            MoveMailToFolder(Statics.Rejected, rejected);
            MoveMailToFolder(Statics.Processed, processed);
        }
    }
}
\$\endgroup\$
6
  • 3
    \$\begingroup\$ Could you please add some more context to your question, like where are the mails coming from (are you using Exchange webservice?), what does ` _service.GetAttachments()` do etc. \$\endgroup\$
    – Heslacher
    Nov 20, 2018 at 6:10
  • 2
    \$\begingroup\$ It's strange to me that _service.GetAttachments(message.Attachments.ToArray(), ... (Attachments is converted to an array) is followed by foreach (var attachment in message.Attachments) (Attachments is used directly). It could be correct, but it looks like non trivial or possibly suspect use of arrays/collections/lists. \$\endgroup\$
    – Peter B
    Nov 20, 2018 at 16:47
  • \$\begingroup\$ thanks for replies. added more code. _service.GetAttachments() will make a call to Microsoft Outlook and retrieve all attachments from a message. hope it helps \$\endgroup\$
    – Gauravsa
    Nov 20, 2018 at 22:45
  • \$\begingroup\$ GetAttachments is from Microsoft API: link here msdn.microsoft.com/en-us/library/office/… \$\endgroup\$
    – Gauravsa
    Nov 20, 2018 at 22:46
  • \$\begingroup\$ If I'm following, there is some strange behavior here. If a message has two PDF attachments, with one being single-page and the other being multi-page, that message will end up in both processed and rejected. Is that correct? \$\endgroup\$
    – benj2240
    Nov 21, 2018 at 19:48

1 Answer 1

2
\$\begingroup\$
  1. The #region Private variables is buying you nothing. It turns your one private variable declaration into three lines, without adding any information. I hate regions in general, and there are exceptions1, but this isn't one of them.

  2. If you expect a List<MailMessage> and I give you a null, you'll do nothing? That's very kind... I would sooner throw an exception, so that the code passing in the nulls can be found and fixed.

  3. If you expect each MailMessage to have a single pdf attachment, and I give you a message has no pdf attachments, or multiple pdf attachments, what will you do? Quietly do nothing? Attempt to do something, but fail in a strange way (for example, attempting to move a message that has already been moved)? I would sooner throw an exception that points to the direct cause. This is easy to do with Linq. Instead of

foreach (var attachment in message.Attachments)
{
    var fileAttach = (FileAttachment) attachment;
    if (fileAttach.Name.EndsWith(".pdf"))
    {
        // handle any pdf attachment
    }
    else
    {
        // ignore any non-pdf attachment
    }
}

You can

var pdfAttachment = message
    .Attachments
    .Cast<FileAttachment>()
    .Single(file => file.Name.EndsWith(".pdf"));

// Handle the single pdf attachment
  1. This may not be as important for you, but it's a good practice to use Path.Combine when constructing file paths. It's arguably simpler, saves you from having to escape strings, prevents possible issues with multiple directory separator characters in a row, and leaves you with cross-platform compatibility.

  2. The main function here accepts a List<MailMessage>. But, the only thing that's ever done with the list is to iterate through it. What if I have a MailMessage[]? As a rule, you should be as accepting as possible of different kinds of input. In this case, IEnumerable<MailMessage> is as specific as you need to be.

  3. The pattern you're using for moving these messages is

Create an empty collection of work items
For each input element:
    Possibly add a work item to the collection
Do all the work in the collection

Unless the "do all work" step is drastically more efficient in batches, I much prefer the simplified

For each input element:
    Possibly do an item of work
  1. For function calls with more than a couple arguments, especially if the parameters you're passing in aren't well-named variables, it's a good idea to use the named parameter syntax. This would make it much clearer, for example, that you aren't passing any additionalProperties to _service.GetAttachments()

  2. PdfReader implements IDisposable, but it's not being disposed. It's a good idea to wrap every disposable object your code uses in a using block, to ensure proper disposal. If that starts to move your code too far to the right, you can address that by moving the code into a separate function.

  3. The code involved in constructing the file name and writing the file to disk is in the same function as the code to iterate through the messages. Those kinds of details should probably be separated out to helper functions. In fact, I would separate this code out into a lot of functions.

Taking all of that into account, here's my rewrite:

public class Client
{
    private ExchangeService _service;

    /// <summary>
    /// Given a collection of messages with exactly one Pdf attachment,
    /// write all single-page pdfs to disk. Any messages with a single-page
    /// pdf attachment will be moved to one folder. Any messages with a
    /// multi-page pdf attachment will be moved to another folder.
    /// </summary>
    public void MoveSinglePagePdfsToFolder(IEnumerable<EmailMessage> messagesWithSinglePdf)
    {
        if (messagesWithSinglePdf is null) throw new ArgumentNullException(nameof(messagesWithSinglePdf));

        foreach (var message in messagesWithSinglePdf)
        {
            MoveSinglePagePdfsToFolder(message);
        }
    }

    private void MoveSinglePagePdfsToFolder(EmailMessage messageWithSinglePdf)
    {
        _service.GetAttachments(
            attachments: messageWithSinglePdf.Attachments.ToArray(),
            bodyType: BodyType.HTML,
            additionalProperties: null);

        var pdfAttachment = messageWithSinglePdf
            .Attachments
            .Cast<FileAttachment>()
            .Where(attachment => attachment.Name.EndsWith(".pdf"))
            .Single(); // throws if the number of pdfs is not exactly one

        if (PageCount(pdfAttachment) == 1)
        {
            SaveToEmaDirectory(pdfAttachment);
            MoveMailToFolder(Statics.Processed, messageWithSinglePdf);
        }
        else
        {
            MoveMailToFolder(Statics.Rejected, messageWithSinglePdf);
        }
    }

    private int PageCount(FileAttachment pdfAttachment)
    {
        using (var reader = new PdfReader(pdfAttachment.Content))
            return reader.NumberOfPages;
    }

    private void SaveToEmaDirectory(FileAttachment singlePagePdf)
    {
        File.WriteAllBytes(
            path: GenerateFilePath(singlePagePdf),
            bytes: UnEditableVersionOf(singlePagePdf.Content));
    }

    private string GenerateFilePath(FileAttachment singlePagepdf)
    {
        return Path.Combine(
            Statics.EmaFileWatcherWorkingDirectory,
            GenerateFileName(singlePagepdf));
    }

    private string GenerateFileName(FileAttachment singlePagePdf)
    {
        return string.Join("_",
            "fax",
            DateTime.Now.Date.ToShortDateString(),
            singlePagePdf.Name,
            DateTime.Now.Ticks + ".pdf");
    }

    private byte[] UnEditableVersionOf(byte[] pdfContent)
    {
        return IsEditablePdf(pdfContent)
            ? GetUnEditablePdf(pdfContent)
            : pdfContent;
    }

1I could go on at length about regions, but I will spare you for now.

\$\endgroup\$
1
  • \$\begingroup\$ have changed ur .Single to .First to get first pdf attachment in message \$\endgroup\$
    – Gauravsa
    Nov 22, 2018 at 1:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.