Loop at specific folder in Outlook to look for a specific title

I created a C# console program to loop through a specific folder in Outlook to look for a specific email subject. When I run it on my computer (multi-core CPU), it didn't use up 100% of the CPU resources. But when I test it on the server (single-core CPU), it uses up 100% of the server's CPU resources.

What can I do to optimize the code?

```
static void Main(string[] args)
{
try
{
string targetFolderName = System.Configuration.ConfigurationSettings.AppSettings["targetFolderName"].ToString().Trim();
string archiveFolderName = System.Configuration.ConfigurationSettings.AppSettings["archiveFolderName"].ToString().Trim();
string companyCode = System.Configuration.ConfigurationSettings.AppSettings["companyCode"].ToString().Trim();

Microsoft.Office.Interop.Outlook.Application outlook = new Microsoft.Office.Interop.Outlook.Application();
Microsoft.Office.Interop.Outlook.NameSpace nameSpace = outlook.GetNamespace("MAPI");
Microsoft.Office.Interop.Outlook.MAPIFolder mapiFolderPurchase = (Microsoft.Office.Interop.Outlook.MAPIFolder)nameSpace.GetDefaultFolder(OlDefaultFolders.olFolderInbox).Parent;
Microsoft.Office.Interop.Outlook.MAPIFolder mapiFolderArchive = (Microsoft.Office.Interop.Outlook.MAPIFolder)outlook.Session.Folders[archiveFolderName].Folders["Inbox"].Parent;

using (SqlConnection sqlConnection = new SqlConnection(ConfigurationSettings.AppSettings["drawingRequestConnectionString"].Trim()))
{
string commandText = "SELECT Regno,UserAccEmail,Rec_date,vendor,approver from SubconJobQ WHERE Status=2 and year(Rec_date)>=2015 ORDER BY Regno DESC";

SqlCommand sqlCommand = new SqlCommand(commandText, sqlConnection);

sqlConnection.Open();

using (SqlDataReader sqlDataReader = sqlCommand.ExecuteReader())
{
if (sqlDataReader.HasRows)
{
while (sqlDataReader.Read())

You used a using for SqlConnection and SqlDataReader, so why not for SqlCommand?

By the time we get to if (sqlDataReader.HasRows) we're already several tabs deep. I'd advise you to move all code inside that if block to a separate method, because it is getting hard to keep an overview.

On a related note: why is your assignment to commandText inside a using inside a try? Define that as a const outside the Main().

Please use var so you'll save some screen estate here:

Microsoft.Office.Interop.Outlook.Application outlook = new Microsoft.Office.Interop.Outlook.Application();
Microsoft.Office.Interop.Outlook.NameSpace nameSpace = outlook.GetNamespace("MAPI");
Microsoft.Office.Interop.Outlook.MAPIFolder mapiFolderPurchase = (Microsoft.Office.Interop.Outlook.MAPIFolder)nameSpace.GetDefaultFolder(OlDefaultFolders.olFolderInbox).Parent;
Microsoft.Office.Interop.Outlook.MAPIFolder mapiFolderArchive = (Microsoft.Office.Interop.Outlook.MAPIFolder)outlook.Session.Folders[archiveFolderName].Folders["Inbox"].Parent;

Also, can't Microsoft.Office.Interop.Outlook be moved to a using at the top, e.g. using Microsoft.Office.Interop.Outlook;? That way you'd end up with something much more manageable:

var outlook = new Application();
var nameSpace = outlook.GetNamespace("MAPI");
var mapiFolderPurchase = (MAPIFolder)nameSpace.GetDefaultFolder(OlDefaultFolders.olFolderInbox).Parent;
var mapiFolderArchive = (MAPIFolder)outlook.Session.Folders[archiveFolderName].Folders["Inbox"].Parent;

I notice there are magical strings in there -- "MAPI", "Inbox" -- but in this case it's probably overkill to define them as const since you don't use them elsewhere.

All your code is in this Main() of your console application. Why not move it to a class file of its own? It looks much nicer, and offers you much more flexibility.

Why are these called repeatedly:

MAPIFolder vendorFolder = mapiFolderPurchase.Folders["Sent Email to Vendor"];
MAPIFolder archiveSentEmailToVendorFolder = mapiFolderArchive.Folders["Sent Email to Vendor"];

I don't see how the contents of these folders are going to change, so why call this over and over again while you're looping through the vendors of each result of an sqlDataReader?

You repeatedly do vendors[i].Trim(). If you need such a result multiple times, assign it to a variable. But even better would be to eliminate this when you assign vendors. Matter of fact, why is this a ";"-separated field in your DB? That's some bad design right there.

There is no else for if (!string.IsNullOrEmpty(vendors[i])), so reduce nesting by doing this:

if (string.IsNullOrEmpty(vendors[i]))
{
   continue;
}

Same with if (mailItem != null) and if (mailItem.Subject != null).

Why did you copy-paste this logic?

foreach (object item in vendorFolder.Items)
{
    MailItem mailItem = item as MailItem;
    if (mailItem != null)
    {
        if (mailItem.Subject != null)
        {
            if (string.Compare(subject, mailItem.Subject.Trim(), StringComparison.OrdinalIgnoreCase) == 0)
            {
                emailFound = true;
                break;
            }
        }
    }
}

foreach (object item in archiveSentEmailToVendorFolder.Items)
{
    MailItem mailItem = item as MailItem;
    if (mailItem != null)
    {
        if (mailItem.Subject != null)
        {
            if (string.Compare(subject, mailItem.Subject.Trim(), StringComparison.OrdinalIgnoreCase) == 0)
            {
                emailFound = true;
                break;
            }
        }
    }
}

This should have been moved to a method:

private bool FindEmail(IEnumerable mailItems)
{
    foreach (var mailItem in mailItems)
    {
        if (mailItem != null)
        {
            if (mailItem.Subject != null)
            {
                if (string.Compare(subject, mailItem.Subject.Trim(), StringComparison.OrdinalIgnoreCase) == 0)
                {
                    return true;
                }
            }
        }
    }
}

Which can be called by:

emailFound = FindEmail(vendorFolder.Items.Cast());

emailFound = FindEmail(archiveSentEmailToVendorFolder.Items.Cast());

I think this could probably be improved upon using LINQ.

I'd rethink your whole approach.

First of all I'd dump all that SqlConnection etc. It's 2015, use Entity Framework. For instance, right now the field "Regno" is used in three places: in the SELECT, in the ORDER BY, and it 's used to find a value: sqlDataReader["Regno"].ToString().Trim(). That's three chances of making a typo and three places you need to update if this field ever is renamed.

Secondly, separate you Outlook lookup from your DB query. Right now you're opening a DB connection, execute a query and loop through the results, and in each iteration you're doing at least one Outlook lookup. What you should do is get the db results as entities and loop through those entities.

StackExchange Code Review Q#104130, answer score: 5