6
\$\begingroup\$

This is my first C# project, so feel free to be brutal. :)

I have an automated external process on another machine generate and email a budget report to me every Sunday. This program is a Microsoft Outlook add-in that automatically fetches those emails and saves the attached report to a particular location on my hard drive organized by year. Here's the algorithm:

  1. if there is no unread email in the mail folder, quit
  2. otherwise, save the attachment to a subfolder under the year
  3. renaming it to append the date

Because it's tied to the new email event, it only runs once each time new a new email is received.

This is the code which includes some autogenerated code.

BudgetTool.cs

using System;
using Outlook = Microsoft.Office.Interop.Outlook;

namespace BudgetToolAddIn
{
    public partial class BudgetTool
    {
        private void BudgetTool_Startup(object sender, System.EventArgs e)
        {
            this.Application.NewMail += new Microsoft.Office.Interop.Outlook
                .ApplicationEvents_11_NewMailEventHandler(ThisApplication_NewMail);
        }

        private void ThisApplication_NewMail()
        {
            string folderName = "Budget Reports";
            // the destiationFolder must include the trailing \
            string destinationFolder = @"C:\users\Edward\planning\budget\";
            Outlook.MAPIFolder inBox = Application.ActiveExplorer()
                    .Session.GetDefaultFolder(Outlook.OlDefaultFolders.olFolderInbox).Parent;
            try
            {
                inBox = inBox.Folders[folderName];
            }
            catch
            {
                System.Windows.Forms.MessageBox.Show(@"Can't find folder '" + folderName + "'");
                return;
            }
            Outlook.Items inBoxItems = inBox.Items;
            Outlook.MailItem newEmail = null;
            inBoxItems = inBoxItems.Restrict("[Unread] = true");
            try
            {
                foreach (object collectionItem in inBoxItems)
                {
                    newEmail = collectionItem as Outlook.MailItem;
                    if (newEmail != null)
                    {
                        if (newEmail.Attachments.Count > 0)
                        {
                            for (int i = 1; i <= newEmail
                               .Attachments.Count; i++)
                            {
                                const string extension = ".csv";
                                string originalFileName = newEmail.Attachments[i].FileName;
                                if (originalFileName.EndsWith(extension))
                                {
                                    int extloc = originalFileName.LastIndexOf(extension);
                                    DateTime date = newEmail.SentOn;
                                    string year = date.ToString("yyyy");
                                    string fileName = originalFileName.Remove(extloc, extension.Length) + "_" + date.ToString("yyyyMMdd") + extension;
                                    newEmail.Attachments[i].SaveAsFile(destinationFolder + year + @"\" + fileName);
                                }
                            }
                        }
                    }
                }
            }
            catch (Exception ex)
            {
                string errorInfo = (string)ex.Message
                    .Substring(0, 11);
                if (errorInfo == "Cannot save")
                {
                    System.Windows.Forms.MessageBox.Show(@"Create Folder <{destionationFolder}>");
                }
            }
        }


        private void BudgetTool_Shutdown(object sender, System.EventArgs e)
        {
            // Note: Outlook no longer raises this event. If you have code that 
            //    must run when Outlook shuts down, see https://go.microsoft.com/fwlink/?LinkId=506785
        }

        #region VSTO generated code

        /// <summary>
        /// Required method for Designer support - do not modify
        /// the contents of this method with the code editor.
        /// </summary>
        private void InternalStartup()
        {
            this.Startup += new System.EventHandler(BudgetTool_Startup);
            this.Shutdown += new System.EventHandler(BudgetTool_Shutdown);
        }

        #endregion
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ WHY U NO USING VBA ;-) \$\endgroup\$ – Phrancis Jul 26 '18 at 20:27
  • 2
    \$\begingroup\$ @Phrancis: OK, that was brutal. ;) \$\endgroup\$ – Edward Jul 26 '18 at 20:54
4
\$\begingroup\$

As it seems to be a "for-your-own-eyes-only"-function I will not comment on the hard coded paths and other strings :-).

This syntax

        this.Application.NewMail += new Microsoft.Office.Interop.Outlook.ApplicationEvents_11_NewMailEventHandler(ThisApplication_NewMail);

has a short form:

this.Application.NewMail += ThisApplication_NewMail;

In

if (newEmail.Attachments.Count > 0)
{
  for (int i = 1; i <= newEmail.Attachments.Count; i++)
  {
    ...

the if (newEmail.Attachments.Count > 0)-check is unnecessary.


This catch statement is somewhat incomplete:

        catch (Exception ex)
        {
            string errorInfo = (string)ex.Message
                .Substring(0, 11);
            if (errorInfo == "Cannot save")
            {
                System.Windows.Forms.MessageBox.Show(@"Create Folder <{destionationFolder}>");
            }
        }

You are only handling one special Exception. What about notifying if anything else going wrong? Why not handle the special Exception for missing folder in a dedicated exception handler?

If the Exception for missing folder is not a sub class of Exception then you could experiment with something like:

  }
  catch (Exception ex) when (ex.Message.Contains("Cannot save"))
  {
    ...
  }
  catch (Exception ex)
  {
    ...
  }

Or maybe it's a better approach to check for the existence of the destination folder before the loop, and if missing exit more "gracefully" (or maybe just create it if the application has the rights to do that)?


You are exposing errors in modal message boxes. What will happen if a message box halts the event progress and a new email arrives?

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't usually write for Windows. What's a better way of handling errors that doesn't use modal message boxes? \$\endgroup\$ – Edward Jul 27 '18 at 11:39
  • \$\begingroup\$ @Edward: The canonical way is to log to the Windows event log - in .net through the System.Diagnostics.EvnetLog api msdn.microsoft.com/en-us/library/xzwc042w(v=vs.110).aspx. Or you could just write to a simple text file. \$\endgroup\$ – Henrik Hansen Jul 27 '18 at 13:22
  • 1
    \$\begingroup\$ I wouldn't recommend using the EventLog even to my worst enemy ;-) \$\endgroup\$ – t3chb0t Jul 27 '18 at 18:00
4
\$\begingroup\$
newEmail = collectionItem as Outlook.MailItem;
if (newEmail != null)
{
}

You can replace this with a nice filter-expression like:

if (collectionItem is Outlook.MailItem newEmail)
{
    ...
}

This way the additional variable declaration

Outlook.MailItem newEmail = null;

is no longer necessary.


originalFileName.EndsWith(extension)

Windows paths are case-insensitive so comparing extensions is better done with

originalFileName.EndsWith(extension, StringComparison.OrdinalIgnoreCase)

originalFileName.Remove(extloc, extension.Length)

You don't have to extract file names and extensions manually. There is a simpler API for handling this. It's hidden in the static Path class:

Path.GetFileNameWithoutExtension(..)

and

Path.GetExtension(..)
\$\endgroup\$
  • 1
    \$\begingroup\$ I suspected there was a better way to handle file paths and extensions, and even searched, but the API is huge and I couldn't find it. Thanks! \$\endgroup\$ – Edward Jul 27 '18 at 11:42
  • 2
    \$\begingroup\$ For what it's worth, I wish I could mark both answers as "accepted." \$\endgroup\$ – Edward Dec 12 '18 at 15:12
  • \$\begingroup\$ @Edward thanks ;-) I know what you mean, I often feel the same. \$\endgroup\$ – t3chb0t Dec 12 '18 at 15:14

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.