Check if mail contains a single page pdf and transfer to other folders

Question

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);
        }
    }
}

Answer 1

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 exceptions¹, 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

4. 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.

5. 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.

6. 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

7. 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()

8. 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.

9. 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.}