3
\$\begingroup\$

I wrote an email syncing program in my ASP.NET MVC website that syncs e-mails from multiple e-mail accounts to my database.

Each e-mail account has to be independent from other accounts and they have to run asynchronously.

Are there are any issues with my code such as efficiency, bugs or can there be made improvements?

   public void Start()
    {
        new Thread(StartSyncing).Start();
    }

   private void StartSyncing()
    {
        foreach (var emailAccount in _emailAccountRepository.GetList())
        {
            // Create a local copy
            EmailAccount copyEmailAccount = emailAccount;

            // Start a new thread for each email account
            new Thread((x => SyncAccount(copyEmailAccount))).Start();
        }
    }

   private void SyncAccount(EmailAccount emailAccount)
    {
        // Sync forever
        while (true)
        {
            // Set the next syncing date
            _nextSyncingDate = DateTime.Now.AddMinutes(5);

            // Create a fresh start for the database context
            InitRepositories();

            // Connect to the mail server
            using (var emailProvider = new ReceiveEmailProvider(emailAccount.Email,
                emailAccount.Password, emailAccount.Host, emailAccount.Port, emailAccount.Secure))
            {
                // For each email
                for (int emailCounter = emailProvider.GetAmountEmails(); emailCounter > 0; emailCounter--)
                {
                    // As long the email is new
                    while (IsNewEmail(emailCounter))
                    {
                        // Get email
                        var email = emailProvider.GetEmail(emailCounter, emailAccount.Id);

                        // Check if the email isn't spam
                        if (!IsSpam(GetEmailDomain(email.FromEmail), emailAccount.User.UserId))
                        {
                            // Add the new email
                            var dbEmail = AddNewEmail(email, emailAccount.Id, emailCounter);

                            // Add email to category
                            AddEmailToCategory(emailAccount.User.UserId, dbEmail);
                        }
                    }
                }
            }
            // Set end starting sync date
            _endSyncingDate = DateTime.Now;

            // Get the difference in time between the end and starting date
            var timeDifference = _nextSyncingDate - _endSyncingDate;

            // Sleep if we haven't wait for 5 minutes
            if (_nextSyncingDate > _endSyncingDate)
            {
                // Sleep till the 5 minutes have passed
                Thread.Sleep((int)timeDifference.TotalMilliseconds);
            }
        }
    }

I added the methods that seem relevant to me. If you require more code, let me know!

\$\endgroup\$
  • 1
    \$\begingroup\$ Why don't you do new ReceiveEmailProvider(emailAccount) instead of new ReceiveEmailProvider(emailAccount.Email, emailAccount.Password, emailAccount.Host, emailAccount.Port, emailAccount.Secure)? Or do you not have control over ReceiveEmailProvider? \$\endgroup\$ – BCdotWEB Oct 24 '14 at 12:50
  • \$\begingroup\$ @BCdotNET I do have control over the receiveEmailProvider. I am using this method because of testing purposes, everything was hardcoded. I'm changing it now! \$\endgroup\$ – Jamie Oct 24 '14 at 12:54
  • 1
    \$\begingroup\$ As it stands, it works. But ideally you want a way to capture exceptions and restart threads. You could create an interface, and implement the inteface for various accounts types etc but that is allot of thinking and code and debugging, and only feasible if you are creating this to be extensible. Otherwise, put throw exception in random places and see how the process handles it, patch it, publish. Done. \$\endgroup\$ – Piotr Kula Oct 24 '14 at 17:26
1
\$\begingroup\$

I feel like some of the code you haven't shown has a few leaky abstractions. For example:

using (var emailProvider = new ReceiveEmailProvider(emailAccount.Email,
            emailAccount.Password, emailAccount.Host, emailAccount.Port, emailAccount.Secure))

Every argument you send to RecieveEmailProvider comes from emailAccount. I would add an overload to it's constructor so that you could simply pass in an emailAccount. I think it greatly simplifies the code.

using (var emailProvider = new RecieveEmailProvider(emailAccount))

There's something similar happening here.

var dbEmail = AddNewEmail(email, emailAccount.Id, emailCounter);

// Add email to category
AddEmailToCategory(emailAccount.User.UserId,dbEmail);

Why does the client code go through all the trouble of spelling this out? AddEmailToCategory should know how to get the info it needs from an emailAccount so the dev doesn't have to.

\$\endgroup\$

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.