Email Service - Sending an email with azure blob storage

Question

Here is some code that sends an email using Mimekit for .net C# to send email via Office365. DI is done using TinyIOC. It downloads an image from blob storage and then sends it with an attachment. Each email will ALWAYS have at least one attachment. Code feels a bit messy, especially the body/subject selector.

/// <summary> Handles emails using Office365 </summary>
        public class EmailService : IEmailService
        {
            /// <summary> Default body of email </summary>
            private static string _body = "Please check system, something went wrong";

        /// <summary> Default subject of email </summary>
        private static string _subject = "Error";

        private readonly IBlobStorageService _blobStorageService;
        private readonly EmailSettings _emailSettings;

        public EmailService(IBlobStorageService blobStorageService, EmailSettings emailSettings)
        {
            _emailSettings = emailSettings ?? throw new ArgumentNullException("Email configuration cannot be null.");

            _blobStorageService = blobStorageService;
        }

        /// <summary> Sends an email using Office365 with the credentials specified in configuration. </summary>
        /// <param name="motionActivity"></param>
        /// <param name="mediaResults"></param>
        /// <returns></returns>
        /// <exception cref="ArgumentException"></exception>
        public async Task<bool> SendEmail(MotionActivity motionActivity, List<MediaAnalysis> mediaResults)
        {
            if (motionActivity == null)
                throw new ArgumentException("Motion activity cannot be null");
            if (mediaResults == null)
                throw new ArgumentException("Media result should not be null");

            // Filter out all results below 60% probability
            var mediaResultsAbove60Percent = mediaResults.FindAll(x => x.Probability >= 60);

            // If no pictures remain afterwards, return
            if (mediaResultsAbove60Percent.Count == 0) return false;

            var highestProbability = mediaResultsAbove60Percent.Max(x => x.Probability);

            var message = await CreateEmailMessage(motionActivity, highestProbability, mediaResultsAbove60Percent);

            var fullMessage = AddRecipients(message, highestProbability);

            return await SendMessageAsync(fullMessage).ConfigureAwait(false);
        }

        /// <summary> Sends the message using MimeMessage/office365 </summary>
        /// <param name="message"> The message we are sending </param>
        /// <returns>Whether or not the message send was successful.</returns>
        private async Task<bool> SendMessageAsync(MimeMessage message)
        {
            try
            {
                using (var client = new SmtpClient())
                {
                    await client.ConnectAsync(_emailSettings.Host, _emailSettings.Port, false);
                    await client.AuthenticateAsync(_emailSettings.FromAddress, _emailSettings.Password);

                    await client.SendAsync(message);
                    await client.DisconnectAsync(true);
                }
            }
            catch (Exception ex)
            {
                Log.Error(
                    "An error occurred trying to send an Office 365 email: {message}, {innerException}, {stacktrace}",
                    ex.Message, ex.InnerException, ex.StackTrace);
                return false;
            }

            return true;
        }

        /// <summary> Creates the templates used for emails </summary>
        /// <param name="motionActivity"> is the data of the entire time we collected motion for ONE camera </param>
        /// <param name="highestProbability"> the highest probability we found that we think there is a canister </param>
        private async Task<MimeMessage> CreateEmailMessage(MotionActivity motionActivity, double highestProbability,
            List<MediaAnalysis> mediaResultsAbove60Percent)
        {
            //Changes subject headline based on probability
            if (highestProbability >= 60 && highestProbability <= 74)
            {
                var auditText =
                    "Dear Auditor please review, " +
                    $"\r\n Images captured: {motionActivity}";
                _subject = $"Detect: Audit Image - {highestProbability}%";
                _body = auditText;
            }

            if (highestProbability >= 75 && highestProbability <= 87)
            {
                var lowProbabilityText =
                    "Dear Concierge, " +
                    "\r\n It is possible that an explosive canister has entered the building at the address above. Please review the attached photographs and take precautionary action." +
                    $"\r\n Images captured: {motionActivity}";

                _subject = $"Detect: Warning Image - {highestProbability}%";
                _body = lowProbabilityText;
            }

            if (highestProbability >= 88 && highestProbability <= 100)
            {
                var highProbabilityText =
                    "Dear Concierge, " +
                    "\r\n It is likely that an explosive canister has entered the building at the address above. Please review the attached photographs and take immediate action." +
                    $"\r\n Images captured: {motionActivity}";

                _subject = $"Detect: High Risk Image - {highestProbability}%";
                _body = highProbabilityText;
            }

            // Construct the email body.
            var builder = new BodyBuilder {HtmlBody = _body};
            var message = new MimeMessage();
            message.From.Add(new MailboxAddress(_emailSettings.DisplayName, _emailSettings.FromAddress));
            message.Subject = _subject;

            var emailWithAttachments = await AddImageAttachment(mediaResultsAbove60Percent, message, builder);

            emailWithAttachments.Body = builder.ToMessageBody();

            return emailWithAttachments;
        }

        /// <summary> Adds all recipients to our email </summary>
        /// <param name="email"> the email we are sending </param>
        /// <param name="highestProbability"></param>
        private MimeMessage AddRecipients(MimeMessage email, double highestProbability)
        {
            if (highestProbability >= 75)
                foreach (var recipient in _emailSettings.FullRecipients)
                    email.To.Add(new MailboxAddress(recipient));
            else
                foreach (var recipient in _emailSettings.Recipients)
                    email.To.Add(new MailboxAddress(recipient));

            return email;
        }

        /// <summary> Takes all images above 60% probability and adds the image to the email</summary>
        /// <param name="mediaResultsAbove60Percent"> images above 60% </param>
        /// <param name="email"> the email we will be sending </param>
        /// <param name="bodyBuilder"> the body builder for emails </param>
        /// <returns></returns>
        private async Task<MimeMessage> AddImageAttachment(IEnumerable<MediaAnalysis> mediaResultsAbove60Percent,
            MimeMessage email, BodyBuilder bodyBuilder)
        {
            foreach (var mediaResult in mediaResultsAbove60Percent)
            {
                if (mediaResult.MediaUrl == null) continue;

                var download = await _blobStorageService.DownloadFile(mediaResult.MediaUrl).ConfigureAwait(false);
                var byteArray = ReadFully(download.Content);

                bodyBuilder.Attachments.Add(mediaResult.MediaUrl, byteArray);
            }

            return email;
        }

        /// <summary> Helper method for add image attachment. Converts stream to byte array. </summary>
        /// <param name="input"> the image we are converting </param>
        /// <returns> the byte array </returns>
        private static byte[] ReadFully(Stream input)
        {
            using (var ms = new MemoryStream())
            {
                input.CopyTo(ms);
                return ms.ToArray();
            }
        }
    }

Answer 1

Some quick remarks:

• if (mediaResultsAbove60Percent.Count == 0) return false; can be replaced by the much more readable return !mediaResultsAbove60Percent.Any();.

• I'd extract parts of CreateEmailMessage to a separate method, or even class of its own, for instance a BodyRetriever class. Right now it does way too much.

• You use bodyBuilder.HtmlBody yet the body you compile is plain text. Instead of using string concatenation, why not design nicer email texts as HTML files, embed those HTML files and read those at runtime? (You can have placeholders in the HTML file and replace those if necessary.)

• I don't see the point in creating auditText, lowProbabilityText and highProbabilityText, considering you immediately assign these to _body.

• Don't use "\r\n", use Environment.NewLine.

• Comments should explain the why, not the what. If // Construct the email body. only applies to the next line -- var builder = new BodyBuilder {HtmlBody = _body}; -- it is pointless, if it applies to the whole block of lines below it, it is incorrect.

• ReadFully() isn't proper English and pretty meaningless. Why not ToByteArray()?

• AddRecipients has duplicate code. Instead, do this: var recipients = highestProbability >= 75 ? _emailSettings.FullRecipients : _emailSettings.Recipients; and then loop through recipients.

• No need to return email; at the end of AddRecipients: this method has already updated MimeMessage email passed as an parameter. Just call AddRecipients(message, highestProbability);, no need to assign the result to var fullMessage.

---

IMHO you should consider splitting up this class into smaller ones, e.g. one to construct the MimeMessage, which itself should call a class to construct the email body etc. I'd rather have a couple of small classes where each does one particular task, than a 200 line "Service" class that contains a lot of private methods etc.

SendEmail should basically call something like ComposeEmail and then SendEmail (please don't add "Async" to the end of methods where there is no need). Quite honestly I'd even extract SendMessageAsync to a class of its own (something like EmailSender). Keep the Service class clean, and simply have it delegate its work to other classes.